store delegated operation IO in metadata #4436
Conversation
WalkthroughThe recent changes introduce enhanced metadata handling and schema management for delegated operations in the FiftyOne codebase. Key updates include adding metadata parameters to various functions, setting and extracting schemas, and modifying unit tests to validate these features. This improves the flexibility and accuracy of operations by allowing detailed schema definitions and metadata to be passed and utilized throughout the execution process. Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant FiftyOneSystem
participant DelegatedOperationQueue
participant Database
User->>FiftyOneSystem: Initiate Operation
FiftyOneSystem->>DelegatedOperationQueue: queue_operation(metadata)
DelegatedOperationQueue->>Database: Store Operation with Metadata
Database-->>DelegatedOperationQueue: Acknowledge Storage
DelegatedOperationQueue->>FiftyOneSystem: Operation Queued
FiftyOneSystem->>DelegatedOperationQueue: set_completed(outputs_schema)
DelegatedOperationQueue->>Database: Update Operation with Output Schema
Database-->>DelegatedOperationQueue: Acknowledge Update
DelegatedOperationQueue->>FiftyOneSystem: Operation Completed
FiftyOneSystem->>User: Notify Completion
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
b278aef to
171035f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- fiftyone/factory/repos/delegated_operation.py (5 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (3 hunks)
- fiftyone/operators/delegated.py (7 hunks)
- fiftyone/operators/executor.py (4 hunks)
- tests/unittests/delegated_operators_tests.py (8 hunks)
Additional comments not posted (15)
fiftyone/factory/repos/delegated_operation_doc.py (2)
52-52: Initialization ofmetadataattribute toNone.This change aligns with the PR's objective to enhance metadata handling by initializing the
metadataattribute in theDelegatedOperationDocumentclass.
107-107: Conditional assignment ofmetadatafrom the document.This ensures that the
metadataattribute is set based on the presence of the "metadata" key in the input dictionary, which is crucial for the dynamic handling of metadata in operations.fiftyone/operators/delegated.py (3)
20-20: Import ofresolve_type_with_context.This import is necessary for the new functionality in the
set_completedmethod where the output schema is resolved.
Line range hint
42-56: Addition ofmetadataparameter inqueue_operation.The inclusion of a
metadataparameter allows passing optional metadata containing input and output schemas, which is essential for the new metadata handling features.
128-145: Handling of output schema resolution inset_completed.This change is crucial for storing the output schema when marking an operation as completed, enhancing the robustness of metadata management.
fiftyone/factory/repos/delegated_operation.py (3)
47-47: Addition ofoutputs_schemaparameter inupdate_run_state.This addition supports the new functionality of updating the operation's run state with an output schema, aligning with the PR's objectives related to schema management.
168-172: Handling ofmetadatainqueue_operation.This ensures that metadata can be optionally included when queuing operations, which is vital for the enhanced metadata handling capabilities introduced in this PR.
Line range hint
231-254: Enhanced handling ofoutputs_schemainupdate_run_state.This modification allows the dynamic updating of the
metadata.outputs_schemabased on the operation's completion state, which is a significant enhancement for schema management.fiftyone/operators/executor.py (2)
222-222: Ensure proper unpacking ofpreparedinexecute_or_delegate_operator.The unpacking of the
preparedtuple to includeinputsaligns with the changes in theprepare_operator_executorfunction, ensuring that input schemas are correctly handled.
379-395: Review the new functionresolve_type_with_context.This function adds flexibility in resolving either the inputs or outputs schema based on the provided context and target. It's a useful addition for dynamic schema resolution based on operation context.
tests/unittests/delegated_operators_tests.py (5)
47-50: Ensure theto_jsonmethod inMockInputsclass correctly represents the expected schema.
52-55: Ensure theto_jsonmethod inMockOutputsclass correctly represents the expected schema.
187-187: The assertionself.assertIsNone(doc.metadata)is correct as per the initial state ofmetadatabeingNone.
189-203: The test correctly handles the scenario wheremetadatais provided and asserts its presence and correctness.
Line range hint
297-334: The testtest_set_run_stateseffectively checks the metadata handling during different states of operation execution. Ensure that themetadataincludes bothinputs_schemaandoutputs_schemacorrectly.
171035f to
0d95304
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (10)
fiftyone/operators/delegated.py (2)
42-42: Ensure proper documentation for themetadataparameter in thequeue_operationmethod.The addition of the
metadataparameter is well-integrated, but consider enhancing the docstring to include possible keys and value types within themetadatadictionary for better clarity.
410-410: Clarify the unpacking of variables in_execute_operator.The unpacking of variables in this method is unclear, especially with placeholders like
_and__. Consider using more descriptive variable names to improve code readability.fiftyone/factory/repos/delegated_operation.py (2)
47-47: Ensure theoutputs_schemaparameter is documented in theupdate_run_statemethod.The addition of the
outputs_schemaparameter is crucial for schema management. Enhance the method's docstring to describe this parameter's role and expected data type.
Line range hint
231-254: Refactor to improve error handling inupdate_run_state.The method uses a bare
exceptwhich is generally discouraged. Specify the exceptions you expect to handle to avoid catching unexpected exceptions.- except: + except SpecificExceptionType:Additionally, enhance the logging to provide more context about the error.
fiftyone/operators/executor.py (4)
Line range hint
281-281: Avoid using bareexceptstatements.Using bare
exceptcan catch unexpected exceptions and obscure programming errors. Specify the exception types to handle them appropriately.- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())
Line range hint
288-288: Avoid using bareexceptstatements.Similar to the previous comment, specify the exception types to improve error handling.
- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())
Line range hint
420-420: Unused local variablee.The variable
eis declared but not used. Consider removing it or using it appropriately.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())
Line range hint
914-914: Avoid inequality comparisons toTrue.Use
if not error.custom:for false checks to improve readability and Pythonic style.- if not error.custom != True: + if not error.custom:tests/unittests/delegated_operators_tests.py (2)
Line range hint
652-652: Avoid using bareexceptstatements as they can catch unexpected exceptions and hide programming errors.- except: + except Exception as e: + # Handle specific exception or log it + print(f"Error occurred: {e}")
Line range hint
694-694: Avoid using bareexceptstatements as they can obscure the source of errors.- except: + except Exception as e: + # Handle specific exception or log it + print(f"Error occurred: {e}")
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- fiftyone/factory/repos/delegated_operation.py (5 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (3 hunks)
- fiftyone/operators/delegated.py (7 hunks)
- fiftyone/operators/executor.py (4 hunks)
- tests/unittests/delegated_operators_tests.py (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/factory/repos/delegated_operation_doc.py
Additional Context Used
Ruff (30)
fiftyone/factory/repos/delegated_operation.py (1)
187-187: Do not use bare
exceptfiftyone/operators/delegated.py (2)
133-133: Do not use bare
except
380-380: Do not use bare
exceptfiftyone/operators/executor.py (6)
160-160: Do not use bare
except
281-281: Do not use bare
except
288-288: Do not use bare
except
375-375: Local variable
eis assigned to but never used
420-420: Local variable
eis assigned to but never used
914-914: Avoid inequality comparisons to
True; useif not error.custom:for false checkstests/unittests/delegated_operators_tests.py (21)
309-309: f-string without any placeholders
363-363: f-string without any placeholders
391-391: f-string without any placeholders
425-425: f-string without any placeholders
459-459: f-string without any placeholders
485-485: f-string without any placeholders
526-526: f-string without any placeholders
563-563: f-string without any placeholders
613-613: f-string without any placeholders
652-652: Do not use bare
except
678-678: f-string without any placeholders
694-694: Do not use bare
except
789-789: f-string without any placeholders
797-797: f-string without any placeholders
805-805: f-string without any placeholders
813-813: f-string without any placeholders
860-860: f-string without any placeholders
982-982: f-string without any placeholders
995-995: f-string without any placeholders
1071-1071: f-string without any placeholders
1091-1091: f-string without any placeholders
Additional comments not posted (8)
fiftyone/operators/delegated.py (1)
66-66: Validate the structure ofmetadatabefore using it.Consider adding validation for the
metadataparameter to ensure it contains the expected structure and data types before proceeding with the operation queuing.fiftyone/factory/repos/delegated_operation.py (1)
168-172: Validate themetadataparameter in thequeue_operationmethod.Consider implementing validation for the
metadataparameter to ensure it adheres to the expected structure and data types. This can prevent potential issues during operation execution.fiftyone/operators/executor.py (1)
222-222: Ensure proper unpacking ofpreparedwith addedinputs.This change aligns with the PR's objective to enhance metadata handling by including input schemas.
tests/unittests/delegated_operators_tests.py (5)
47-50: TheMockInputsclass correctly implements theto_jsonmethod.
52-54: TheMockOutputsclass correctly implements theto_jsonmethod.
87-91: TheMockOperatorWithIOclass correctly implements theresolve_inputandresolve_outputmethods to return instances ofMockInputsandMockOutputs, respectively.
192-192: Thequeue_operationmethod inDelegatedOperationServiceTestscorrectly handles metadata, ensuring it is set and retrieved as expected. This aligns with the PR's objective to enhance metadata handling.Also applies to: 194-208
Line range hint
302-340: The testtest_set_run_stateseffectively verifies the metadata handling by setting and checkinginputs_schemaandoutputs_schema. This is crucial for ensuring the integrity of the metadata functionality.
fiftyone/operators/executor.py
Outdated
| @@ -366,6 +376,24 @@ async def resolve_type(registry, operator_uri, request_params): | |||
| return ExecutionResult(error=traceback.format_exc()) | |||
|
|
|||
|
|
|||
| def resolve_type_with_context(context, target: str = None): | |||
There was a problem hiding this comment.
Unused local variable e.
The variable e is declared but not used. Consider removing it if it's unnecessary, or use it appropriately if needed.
- except Exception as e:
+ except Exception:
return ExecutionResult(error=traceback.format_exc())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def resolve_type_with_context(context, target: str = None): | |
| def resolve_type_with_context(context, target: str = None): | |
| try: | |
| # Some code that might raise an exception | |
| pass | |
| except Exception: | |
| return ExecutionResult(error=traceback.format_exc()) |
fiftyone/operators/delegated.py
Outdated
| outputs_schema = None | ||
| try: | ||
| doc = self._repo.get(_id=doc_id) | ||
| outputs = resolve_type_with_context(doc.context, "outputs") | ||
| outputs_schema = outputs.to_json() | ||
| except: | ||
| logger.warning( | ||
| "Failed to resolve output schema for the operation." | ||
| + traceback.format_exc(), | ||
| ) | ||
|
|
||
| return self._repo.update_run_state( | ||
| _id=doc_id, | ||
| run_state=ExecutionRunState.COMPLETED, | ||
| result=result, | ||
| progress=progress, | ||
| run_link=run_link, | ||
| outputs_schema=outputs_schema, |
There was a problem hiding this comment.
Refactor to improve error handling and logging in set_completed.
The current implementation uses a bare except which is generally discouraged as it can catch unexpected exceptions. Consider specifying the exceptions you expect to handle.
- except:
+ except SpecificExceptionType:Also, enhance the logging to include more context about the error.
Committable suggestion was skipped due low confidence.
0d95304 to
37f5f2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (9)
fiftyone/operators/delegated.py (2)
Line range hint
42-56: Add detailed documentation for themetadataparameter.The
metadataparameter in thequeue_operationmethod could benefit from more detailed documentation, especially explaining whatinputs_schemaandoutputs_schemaare expected to contain and how they are used.
Line range hint
380-380: Avoid using bareexceptinexecute_operation.Using a bare
exceptcan catch unexpected exceptions and obscure the source of errors. Specify the exceptions that are expected to be handled.- except: + except ExpectedExceptionType:fiftyone/factory/repos/delegated_operation.py (2)
47-47: Ensureoutputs_schemais documented in theupdate_run_statemethod.The
outputs_schemaparameter is added to theupdate_run_statemethod but lacks documentation. It's important to describe its purpose, expected content, and how it interacts with other parameters or affects the method's behavior.
Line range hint
187-187: Handle potential exceptions more gracefully inqueue_operation.The method uses a bare
exceptwhich might suppress important errors and make debugging difficult. It's advisable to catch specific exceptions or at least log the error appropriately.- except: + except Exception as e: + logger.error("Failed to resolve dataset_id: %s", str(e))fiftyone/operators/executor.py (4)
Line range hint
281-281: Avoid using bareexceptstatements.Using bare
exceptstatements can catch unexpected exceptions and obscure underlying bugs. Specify the exception type to improve error handling.- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())
Line range hint
288-288: Avoid using bareexceptstatements.Similar to the previous comment, specify the exception type to catch specific errors and improve the clarity and maintainability of the code.
- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())
Line range hint
420-420: Unused local variablee.The variable
eis declared but not used in theresolve_execution_optionsfunction. It's good practice to either use the variable or not include it in the exception clause.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())
Line range hint
914-914: Avoid inequality comparisons toTrue.For clarity and consistency, use
if not error.custom:instead of checking inequality toTrue.- if not error.custom != True: + if not error.custom:tests/unittests/delegated_operators_tests.py (1)
Line range hint
652-652: Using bareexceptstatements can lead to masking unexpected errors and make debugging more difficult. It's recommended to catch specific exceptions.- except: + except ExpectedException: # handle specific exceptionAlso applies to: 694-694
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- fiftyone/factory/repos/delegated_operation.py (6 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (3 hunks)
- fiftyone/operators/delegated.py (7 hunks)
- fiftyone/operators/executor.py (4 hunks)
- tests/unittests/delegated_operators_tests.py (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/factory/repos/delegated_operation_doc.py
Additional Context Used
Ruff (30)
fiftyone/factory/repos/delegated_operation.py (1)
187-187: Do not use bare
exceptfiftyone/operators/delegated.py (2)
133-133: Do not use bare
except
380-380: Do not use bare
exceptfiftyone/operators/executor.py (6)
160-160: Do not use bare
except
281-281: Do not use bare
except
288-288: Do not use bare
except
375-375: Local variable
eis assigned to but never used
420-420: Local variable
eis assigned to but never used
914-914: Avoid inequality comparisons to
True; useif not error.custom:for false checkstests/unittests/delegated_operators_tests.py (21)
309-309: f-string without any placeholders
363-363: f-string without any placeholders
391-391: f-string without any placeholders
425-425: f-string without any placeholders
459-459: f-string without any placeholders
485-485: f-string without any placeholders
526-526: f-string without any placeholders
563-563: f-string without any placeholders
613-613: f-string without any placeholders
652-652: Do not use bare
except
678-678: f-string without any placeholders
694-694: Do not use bare
except
789-789: f-string without any placeholders
797-797: f-string without any placeholders
805-805: f-string without any placeholders
813-813: f-string without any placeholders
860-860: f-string without any placeholders
982-982: f-string without any placeholders
995-995: f-string without any placeholders
1071-1071: f-string without any placeholders
1091-1091: f-string without any placeholders
Additional comments not posted (7)
fiftyone/operators/executor.py (2)
222-222: Ensure unpacking ofpreparedmatches expected structure.The unpacking of the
preparedtuple now includesinputs, which aligns with the changes made to theprepare_operator_executorfunction to return these additional parameters. This is a necessary change to handle the new metadata requirements.
379-379: Unused local variablee.The variable
eis declared but not used. Consider removing it if it's unnecessary, or use it appropriately if needed.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())Likely invalid or redundant comment.
tests/unittests/delegated_operators_tests.py (5)
47-50: Theto_jsonmethod inMockInputsclass correctly returns a JSON representation of inputs. This is consistent with the changes described in the PR.
52-55: Theto_jsonmethod inMockOutputsclass correctly returns a JSON representation of outputs. This is consistent with the changes described in the PR.
87-91: Theresolve_inputandresolve_outputmethods inMockOperatorWithIOclass correctly return instances ofMockInputsandMockOutputs, respectively. This aligns with the changes described in the PR and addresses previous review comments.
192-192: The metadata handling intest_delegate_operationmethod is correctly implemented. The test verifies that metadata isNonewhen not provided and correctly matches the provided metadata when it is supplied.Also applies to: 207-208
Line range hint
302-340: Thetest_set_run_statesmethod correctly tests the metadata handling forinputs_schemaandoutputs_schema. The assertions verify that the metadata is stored and retrieved as expected, which is crucial for the functionality introduced in this PR.
fiftyone/operators/delegated.py
Outdated
| outputs_schema = None | ||
| try: | ||
| doc = self._repo.get(_id=doc_id) | ||
| outputs = resolve_type_with_context(doc.context, "outputs") | ||
| outputs_schema = outputs.to_json() | ||
| except: | ||
| logger.warning( | ||
| "Failed to resolve output schema for the operation." | ||
| + traceback.format_exc(), | ||
| ) |
There was a problem hiding this comment.
Refactor to improve error handling and logging in set_completed.
The current implementation uses a bare except which is generally discouraged as it can catch unexpected exceptions. Consider specifying the exceptions you expect to handle.
- except:
+ except SpecificExceptionType:Also, enhance the logging to include more context about the error.
Committable suggestion was skipped due low confidence.
| mock_load_dataset.return_value = MockDataset() | ||
| mock_get_operator.return_value = MockOperatorWithIO() | ||
| doc = self.svc.queue_operation( | ||
| operator="@voxelfiftyone/operator/foo", | ||
| label=mock_get_operator.return_value.name, | ||
| delegation_target=f"test_target", |
There was a problem hiding this comment.
Several f-strings in the test methods do not contain placeholders, which suggests they are unnecessary. Consider removing the f-string prefix if no variables are being interpolated.
- f"test_dataset_{dataset_id}"
+ "test_dataset_{dataset_id}"Also applies to: 363-363, 391-391, 425-425, 459-459, 485-485, 526-526, 563-563, 613-613, 678-678, 789-789, 797-797, 805-805, 813-813, 860-860, 982-982, 995-995, 1071-1071, 1091-1091
Committable suggestion was skipped due low confidence.
| metadata = {"inputs_schema": None, "outputs_schema": None} | ||
|
|
||
| try: | ||
| metadata["inputs_schema"] = inputs.to_json() | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"Failed to resolve inputs schema for the operation: {str(e)}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Handle potential exceptions when converting inputs to JSON.
Consider initializing metadata with default values or ensuring it's always defined before use to avoid potential KeyError issues. Also, ensure that the metadata handling logic is robust against exceptions when converting inputs to JSON.
- metadata = {"inputs_schema": None, "outputs_schema": None}
+ metadata = {"inputs_schema": None, "outputs_schema": None} # Default value to ensure key existence
try:
metadata["inputs_schema"] = inputs.to_json()
except Exception as e:
logger.warning(f"Failed to resolve inputs schema for the operation: {str(e)}")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| metadata = {"inputs_schema": None, "outputs_schema": None} | |
| try: | |
| metadata["inputs_schema"] = inputs.to_json() | |
| except Exception as e: | |
| logger.warning( | |
| f"Failed to resolve inputs schema for the operation: {str(e)}" | |
| ) | |
| metadata = {"inputs_schema": None, "outputs_schema": None} # Default value to ensure key existence | |
| try: | |
| metadata["inputs_schema"] = inputs.to_json() | |
| except Exception as e: | |
| logger.warning( | |
| f"Failed to resolve inputs schema for the operation: {str(e)}" | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
fiftyone/operators/executor.py (4)
Line range hint
281-281: Avoid using bareexceptstatements.- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())Using a bare
exceptcan catch unexpected events like memory errors, interrupts, system exit, and so on. Always specify the exception type to prevent catching unintended exceptions.
Line range hint
288-288: Avoid using bareexceptstatements.- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())Similar to the previous comment, specify the exception type to ensure that only expected errors are caught.
Line range hint
420-420: Unused local variablee.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())Again, the variable
eis declared but not used. Removing it can clean up the code and avoid confusion.
Line range hint
914-914: Avoid inequality comparisons toTrue; useif not error.custom:for false checks.- if not error.custom != True: + if not error.custom:This change simplifies the condition and improves readability.
Review Details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/operators/executor.py (4 hunks)
Additional Context Used
Ruff (6)
fiftyone/operators/executor.py (6)
160-160: Do not use bare
except
281-281: Do not use bare
except
288-288: Do not use bare
except
375-375: Local variable
eis assigned to but never used
420-420: Local variable
eis assigned to but never used
914-914: Avoid inequality comparisons to
True; useif not error.custom:for false checks
Additional comments not posted (2)
fiftyone/operators/executor.py (2)
255-263: Handle potential exceptions when converting inputs to JSON.- metadata = {"inputs_schema": None, "outputs_schema": None} + metadata = {"inputs_schema": None, "outputs_schema": None} # Default value to ensure key existence try: metadata["inputs_schema"] = inputs.to_json() except Exception as e: logger.warning(f"Failed to resolve inputs schema for the operation: {str(e)}")Ensure that
metadatais always defined before use to avoid potentialKeyErrorissues. Also, consider initializingmetadatawith default values.Likely invalid or redundant comment.
379-379: Unused local variablee.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())The variable
eis declared but not used. Consider removing it if it's unnecessary, or use it appropriately if needed.Likely invalid or redundant comment.
| @@ -219,7 +219,7 @@ async def execute_or_delegate_operator( | |||
| if isinstance(prepared, ExecutionResult): | |||
| raise prepared.to_exception() | |||
| else: | |||
| operator, executor, ctx = prepared | |||
| operator, executor, ctx, inputs = prepared | |||
There was a problem hiding this comment.
Ensure proper unpacking of prepared with clear variable names.
- operator, executor, ctx, inputs = prepared
+ operator, executor, ctx, inputs = prepared # Ensure these variables are correctly mapped from `prepared`Consider adding a comment or assertion to ensure that the unpacking aligns with the expected structure of prepared.
Committable suggestion was skipped due low confidence.
|
Teams PR has conflicts. More info here 🙏 |
|
This will require a manual merge and conflict resolution. Any objection to ignoring |
Sure, the check isn't required. But if a Trying to avoid things hitting |
132309f to
d7421c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (6)
fiftyone/factory/repos/delegated_operation.py (1)
Line range hint
187-187: Avoid using bareexceptas it can catch unexpected exceptions. Specify the exceptions you expect to handle.- except: + except SpecificExceptionType:Also, enhance the logging to include more context about the error.
fiftyone/operators/executor.py (5)
Line range hint
281-281: Avoid using bareexceptstatements.- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())
Line range hint
288-288: Avoid using bareexceptstatements.- except: + except Exception as e: return ExecutionResult(executor=executor, error=traceback.format_exc())
Line range hint
375-375: Unused local variablee.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())
Line range hint
420-420: Unused local variablee.- except Exception as e: + except Exception: return ExecutionResult(error=traceback.format_exc())
Line range hint
914-914: Avoid inequality comparisons toTrue; useif not error.custom:for false checks.- if not error.custom != True: + if not error.custom:
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- fiftyone/factory/repos/delegated_operation.py (6 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (3 hunks)
- fiftyone/operators/delegated.py (7 hunks)
- fiftyone/operators/executor.py (4 hunks)
- fiftyone/operators/server.py (1 hunks)
- tests/unittests/delegated_operators_tests.py (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/factory/repos/delegated_operation_doc.py
Additional context used
Ruff
fiftyone/operators/delegated.py
135-135: Do not use bare
except(E722)
382-382: Do not use bare
except(E722)fiftyone/factory/repos/delegated_operation.py
187-187: Do not use bare
except(E722)fiftyone/operators/executor.py
160-160: Do not use bare
except(E722)
281-281: Do not use bare
except(E722)
288-288: Do not use bare
except(E722)
375-375: Local variable
eis assigned to but never used (F841)
420-420: Local variable
eis assigned to but never used (F841)
914-914: Avoid inequality comparisons to
True; useif not error.custom:for false checks (E712)tests/unittests/delegated_operators_tests.py
309-309: f-string without any placeholders (F541)
363-363: f-string without any placeholders (F541)
391-391: f-string without any placeholders (F541)
425-425: f-string without any placeholders (F541)
459-459: f-string without any placeholders (F541)
485-485: f-string without any placeholders (F541)
526-526: f-string without any placeholders (F541)
563-563: f-string without any placeholders (F541)
613-613: f-string without any placeholders (F541)
652-652: Do not use bare
except(E722)
678-678: f-string without any placeholders (F541)
694-694: Do not use bare
except(E722)
789-789: f-string without any placeholders (F541)
797-797: f-string without any placeholders (F541)
805-805: f-string without any placeholders (F541)
813-813: f-string without any placeholders (F541)
860-860: f-string without any placeholders (F541)
982-982: f-string without any placeholders (F541)
995-995: f-string without any placeholders (F541)
1071-1071: f-string without any placeholders (F541)
1091-1091: f-string without any placeholders (F541)
Additional comments not posted (13)
fiftyone/operators/server.py (1)
Line range hint
1-1: No specific changes to review in this file. Ensure that changes in other files are consistent with the usage here.fiftyone/operators/delegated.py (3)
Line range hint
42-56: The addition ofmetadataparameter inqueue_operationmethod is consistent with the PR's objective to enhance metadata handling.
20-20: Ensureresolve_type_with_contextis properly imported and used, as it's a new addition to the imports.Verification successful
The function
resolve_type_with_contextis properly imported and used in the codebase.
- Import:
fiftyone/operators/delegated.py- Definition:
fiftyone/operators/executor.py- Usage:
fiftyone/operators/delegated.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `resolve_type_with_context` in the codebase. # Test: Search for the function usage. Expect: Proper usage and import. rg --type python $'resolve_type_with_context'Length of output: 75
Script:
#!/bin/bash # Description: Verify the usage of `resolve_type_with_context` in the codebase. # Test: Search for the function usage. Expect: Proper usage and import. rg --type py $'resolve_type_with_context'Length of output: 298
147-147: Ensure thatoutputs_schemais correctly handled and integrated into the system, as it's a new addition.Verification successful
Ensure that
outputs_schemais correctly handled and integrated into the system, as it's a new addition.
tests/unittests/delegated_operators_tests.py: The test verifies thatoutputs_schemais correctly set in the metadata.fiftyone/operators/executor.py:outputs_schemais initialized and potentially set within a try block.fiftyone/operators/delegated.py:outputs_schemais documented, initialized, and set based on resolved outputs.fiftyone/factory/repos/delegated_operation.py:outputs_schemais used in method parameters and conditionally updated in the metadata.These occurrences indicate that
outputs_schemais properly handled and integrated into the system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of `outputs_schema` in the codebase. # Test: Search for the usage of `outputs_schema`. Expect: Proper handling and integration. rg --type python $'outputs_schema'Length of output: 64
Script:
#!/bin/bash # Description: Verify the handling of `outputs_schema` in the codebase. # Test: Search for the usage of `outputs_schema`. Expect: Proper handling and integration. rg --type py 'outputs_schema'Length of output: 990
Script:
#!/bin/bash # Description: Extract lines around the occurrences of `outputs_schema` for detailed inspection. # Extract lines from tests/unittests/delegated_operators_tests.py rg --context 5 'outputs_schema' tests/unittests/delegated_operators_tests.py # Extract lines from fiftyone/operators/executor.py rg --context 5 'outputs_schema' fiftyone/operators/executor.py # Extract lines from fiftyone/operators/delegated.py rg --context 5 'outputs_schema' fiftyone/operators/delegated.py # Extract lines from fiftyone/factory/repos/delegated_operation.py rg --context 5 'outputs_schema' fiftyone/factory/repos/delegated_operation.pyLength of output: 3386
fiftyone/factory/repos/delegated_operation.py (1)
47-47: The addition ofoutputs_schemaparameter inupdate_run_statemethod aligns with the PR's objective to enhance metadata handling.fiftyone/operators/executor.py (3)
222-222: Ensure proper unpacking ofpreparedwith clear variable names.
255-263: Handle potential exceptions when converting inputs to JSON.
379-395: Add a new functionresolve_type_with_contextto handle schema resolution.tests/unittests/delegated_operators_tests.py (5)
47-55: LGTM! The newMockInputsandMockOutputsclasses withto_jsonmethods are well-implemented.
86-92: LGTM! TheMockOperatorWithIOclass correctly implementsresolve_inputandresolve_outputmethods to return instances ofMockInputsandMockOutputs.
194-208: The handling ofmetadatainqueue_operationis correctly implemented, ensuring that it is passed and stored as expected.
Line range hint
302-340: The tests for setting run states with metadata handling are well-implemented. The assertions correctly check themetadatavalues against expected schemas.Tools
Ruff
309-309: f-string without any placeholders (F541)
192-192: Ensuremetadatais handled correctly when not provided.Verification successful
The search results show that
doc.metadatais being checked forNoneand compared against expected values in the test cases. This indicates that the code is handling the absence of metadata correctly.
self.assertIsNone(doc.metadata)ensures thatmetadatacan beNone.self.assertEqual(doc.metadata, {"inputs_schema": mock_inputs.to_json()})and similar assertions ensure thatmetadatais correctly set and validated when provided.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the absence of metadata does not cause issues in other parts of the codebase. # Test: Search for the function usage. Expect: Proper handling of `None` metadata. rg --type python $'doc.metadata'Length of output: 62
Script:
#!/bin/bash # Description: Verify that the absence of metadata does not cause issues in other parts of the codebase. # Test: Search for the function usage. Expect: Proper handling of `None` metadata. rg --type py $'doc.metadata'Length of output: 295
Script:
#!/bin/bash # Description: Extract lines around occurrences of `doc.metadata` to verify proper handling of `None` metadata. # Extract 5 lines before and after each occurrence of `doc.metadata` in the specified file. rg --type py --context 5 $'doc.metadata' tests/unittests/delegated_operators_tests.pyLength of output: 1267
| outputs_schema = None | ||
| try: | ||
| doc = self._repo.get(_id=doc_id) | ||
| outputs = asyncio.run( | ||
| resolve_type_with_context(doc.context, "outputs") | ||
| ) | ||
| outputs_schema = outputs.to_json() | ||
| except: | ||
| logger.warning( | ||
| "Failed to resolve output schema for the operation." | ||
| + traceback.format_exc(), | ||
| ) |
There was a problem hiding this comment.
Avoid using bare except as it can catch unexpected exceptions. Specify the exceptions you expect to handle.
- except:
+ except SpecificExceptionType:Also, enhance the logging to include more context about the error.
Committable suggestion was skipped due to low confidence.
Tools
Ruff
135-135: Do not use bare
except(E722)
| # also set the metadata (not required) | ||
| metadata = kwargs.get("metadata", None) | ||
| if metadata: | ||
| setattr(op, "metadata", metadata) | ||
|
|
There was a problem hiding this comment.
Handling of metadata in queue_operation method should ensure it does not overwrite existing metadata if present. Consider merging with existing metadata or explicitly handling cases where metadata might already exist.
+ if metadata:
+ existing_metadata = getattr(op, 'metadata', {})
+ merged_metadata = {**existing_metadata, **metadata}
+ setattr(op, 'metadata', merged_metadata)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| # also set the metadata (not required) | |
| metadata = kwargs.get("metadata", None) | |
| if metadata: | |
| setattr(op, "metadata", metadata) | |
| # also set the metadata (not required) | |
| metadata = kwargs.get("metadata", None) | |
| if metadata: | |
| existing_metadata = getattr(op, 'metadata', {}) | |
| merged_metadata = {**existing_metadata, **metadata} | |
| setattr(op, 'metadata', merged_metadata) |
What changes are proposed in this pull request?
Store delegated operation IO schema in metadata
How is this patch tested? If it is not, please explain why.
Unit and manual testins
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
Chores
ExecutionContextimport to clean up the codebase.