add scheduled state for delegated_ops collection #4810
Conversation
WalkthroughThe pull request introduces modifications to the command-line interface (CLI) documentation and the codebase to incorporate a new operation state, 'SCHEDULED', and its associated sorting option, 'SCHEDULED_AT'. Updates include changes to help text for command-line arguments, the addition of new methods for managing operation states, and enhancements to the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
CamronStaley
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
fiftyone/factory/repos/delegated_operation_doc.py (1)
72-72: Consider usingdoc.get("pending_at", None)for conciseness.The static analysis tool suggests using
doc.get("pending_at", None)instead of anifblock. This is a more concise way to extract thepending_atvalue from the document dictionary and default toNoneif it doesn't exist.Apply this diff to refactor the code:
-self.pending_at = doc["pending_at"] if "pending_at" in doc else None +self.pending_at = doc.get("pending_at", None)Tools
Ruff
72-72: Use
doc.get("pending_at", None)instead of anifblockReplace with
doc.get("pending_at", None)(SIM401)
fiftyone/operators/executor.py (1)
Line range hint
1-1: Annotate the remaining changes for review.The rest of the changes in this file are not annotated with line numbers. Please annotate the remaining changes so they can be thoroughly reviewed.
fiftyone/core/cli.py (1)
Line range hint
12-24: Consider adjusting the fee structure or discount percentages.The current implementation of adding a flat $20 fee to any discounted bill could negate the discount benefit, especially for smaller purchases or lower loyalty tiers. For example, a $100 purchase with a 10% discount for 3 years of loyalty would become $110 after the fee, which is more than the original price.
This might frustrate customers who barely qualify for a discount tier, as the fee could cost them more than the discount saves them. It may also disincentivize customers from maintaining loyalty if the discounts are not worthwhile after the fee.
Consider revising either the discount percentages or the flat fee structure to better align with the goal of rewarding customer loyalty. The fees and discounts should combine to benefit the customer.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- docs/source/cli/index.rst (1 hunks)
- fiftyone/core/cli.py (2 hunks)
- fiftyone/factory/init.py (1 hunks)
- fiftyone/factory/repos/delegated_operation.py (3 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (2 hunks)
- fiftyone/operators/delegated.py (2 hunks)
- fiftyone/operators/executor.py (1 hunks)
Additional context used
Ruff
fiftyone/factory/repos/delegated_operation_doc.py
72-72: Use
doc.get("pending_at", None)instead of anifblockReplace with
doc.get("pending_at", None)(SIM401)
Additional comments not posted (12)
fiftyone/factory/__init__.py (1)
15-15: LGTM!The addition of the
PENDING_ATconstant is a valid extension to theSortByFieldclass. It expands the sorting options for delegated operations, allowing them to be sorted based on their pending status. This change is consistent with the existing constants and is unlikely to introduce any issues.fiftyone/factory/repos/delegated_operation_doc.py (1)
49-49: LGTM!The initialization of the
pending_atattribute toNonein the constructor is a valid way to declare a new optional attribute.fiftyone/operators/delegated.py (3)
104-113: LGTM!The
set_pendingmethod is implemented correctly and follows the existing coding style. The method is properly documented, and the logic is straightforward.
249-261: LGTM!The
get_pending_operationsmethod is implemented correctly and follows the existing coding style. The method is properly documented, and the logic is straightforward. The optional filtering parameters provide flexibility in querying the pending operations.
263-275: LGTM!The
get_running_operationsmethod is implemented correctly and follows the existing coding style. The method is properly documented, and the logic is straightforward. The optional filtering parameters provide flexibility in querying the running operations.fiftyone/factory/repos/delegated_operation.py (5)
67-73: LGTM!The function declaration looks good. It follows the existing pattern of defining a base method that subclasses must implement.
75-81: LGTM!The function declaration looks good. It follows the existing pattern of defining a base method that subclasses must implement.
286-289: Looks good!The updated condition correctly handles the new
PENDINGstate alongside theRUNNINGstate.
363-372: Great implementation!The
get_pending_operationsmethod leverages the existinglist_operationsmethod to retrieve pending operations, promoting code reuse and consistency.
374-383: Great implementation!The
get_running_operationsmethod leverages the existinglist_operationsmethod to retrieve running operations, promoting code reuse and consistency.fiftyone/operators/executor.py (1)
40-40: Looks good! The newPENDINGstate is a helpful addition.The
PENDINGstate provides more granular insight into the operation lifecycle by representing the state where an operation has been triggered but is not yet running. This will allow for better tracking and management of operations.docs/source/cli/index.rst (1)
992-994: LGTM!The addition of the 'PENDING' state and 'PENDING_AT' sort option to the
fiftyone delegated listcommand looks good. It expands the filtering and sorting capabilities without impacting existing usage.
CamronStaley
force-pushed
the
scheduler/pending
branch
from
9408726 to
dd1ea7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
fiftyone/factory/repos/delegated_operation_doc.py (1)
72-72: Consider simplifying the code using thegetmethod.The code segment correctly extracts the
pending_atvalue from the document dictionary and assigns it to thepending_atattribute of the class instance.As suggested by the static analysis hint, you can simplify the code by using the
getmethod of the dictionary:self.pending_at = doc.get("pending_at", None)This will provide a default value of
Noneif the"pending_at"key does not exist in the dictionary, avoiding the need for an explicitifblock.Tools
Ruff
72-72: Use
doc.get("pending_at", None)instead of anifblockReplace with
doc.get("pending_at", None)(SIM401)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- docs/source/cli/index.rst (1 hunks)
- fiftyone/core/cli.py (2 hunks)
- fiftyone/factory/init.py (1 hunks)
- fiftyone/factory/repos/delegated_operation.py (3 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (2 hunks)
- fiftyone/operators/delegated.py (2 hunks)
- fiftyone/operators/executor.py (1 hunks)
Files skipped from review due to trivial changes (1)
- fiftyone/factory/init.py
Files skipped from review as they are similar to previous changes (5)
- docs/source/cli/index.rst
- fiftyone/core/cli.py
- fiftyone/factory/repos/delegated_operation.py
- fiftyone/operators/delegated.py
- fiftyone/operators/executor.py
Additional context used
Ruff
fiftyone/factory/repos/delegated_operation_doc.py
72-72: Use
doc.get("pending_at", None)instead of anifblockReplace with
doc.get("pending_at", None)(SIM401)
Additional comments not posted (1)
fiftyone/factory/repos/delegated_operation_doc.py (1)
49-49: LGTM!The initialization of the
pending_atattribute toNonein the constructor is a valid way to indicate that the attribute is not set initially. The attribute name follows the naming convention used for other timestamp attributes in the class.
CamronStaley
force-pushed
the
scheduler/pending
branch
from
dd1ea7a to
fd78b12
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
fiftyone/factory/repos/delegated_operation_doc.py (1)
72-72: Consider refactoring the code to usedoc.get("pending_at", None).The current code correctly extracts the
pending_atvalue from the document dictionary if it exists, otherwise sets it toNone. However, as suggested by the static analysis hint, usingdoc.get("pending_at", None)is a more concise and Pythonic way to achieve the same result. This would improve the readability of the code.Apply this diff to refactor the code:
-self.pending_at = doc["pending_at"] if "pending_at" in doc else None +self.pending_at = doc.get("pending_at", None)Tools
Ruff
72-72: Use
doc.get("pending_at", None)instead of anifblockReplace with
doc.get("pending_at", None)(SIM401)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- docs/source/cli/index.rst (1 hunks)
- fiftyone/core/cli.py (2 hunks)
- fiftyone/factory/init.py (1 hunks)
- fiftyone/factory/repos/delegated_operation.py (3 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (2 hunks)
- fiftyone/operators/delegated.py (2 hunks)
- fiftyone/operators/executor.py (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- docs/source/cli/index.rst
- fiftyone/core/cli.py
- fiftyone/factory/init.py
- fiftyone/factory/repos/delegated_operation.py
- fiftyone/operators/delegated.py
- fiftyone/operators/executor.py
Additional context used
Ruff
fiftyone/factory/repos/delegated_operation_doc.py
72-72: Use
doc.get("pending_at", None)instead of anifblockReplace with
doc.get("pending_at", None)(SIM401)
Additional comments not posted (1)
fiftyone/factory/repos/delegated_operation_doc.py (1)
49-49: LGTM!The initialization of the
pending_atattribute toNonein the constructor is a valid way to indicate that the attribute is not set initially. The attribute name clearly conveys its purpose of tracking a pending state timestamp.
| doc["completed_at"] if "completed_at" in doc else None | ||
| ) | ||
| self.failed_at = doc["failed_at"] if "failed_at" in doc else None | ||
| self.pending_at = doc["pending_at"] if "pending_at" in doc else None | ||
| self.pinned = doc["pinned"] if "pinned" in doc else None | ||
| self.dataset_id = doc["dataset_id"] if "dataset_id" in doc else None | ||
| self.run_link = doc["run_link"] if "run_link" in doc else None |
There was a problem hiding this comment.
nit: I agree with a suggestion from coderabbit, can we just simply replace all of these doc["thing"] if "thing" in doc else None with doc.get("thing")
There was a problem hiding this comment.
Agree that the 'if thing in doc' syntax should be simplified to just 'get' but is it possible doc is None and if so, should it throw? If not maybe what's needed here is just a 'if doc'?
There was a problem hiding this comment.
I don't think it's possible. It would throw before and it would throw now
lol @coderabbitai this is a wild hallucination, |
|
Tip For best results, initiate chat on the files or code changes. @swheaton, I apologize for the confusion earlier. It seems my previous comment was not relevant to (_/) Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
fiftyone/factory/repos/delegated_operation_doc.py (2)
57-57: Looks good! Here's a minor suggestion.Using
doc.get("operator")is a cleaner way to retrieve the value. As a small nitpick, you can remove theNonedefault value sincegetalready returnsNoneif the key is missing.-self.operator = doc.get("operator") +self.operator = doc.get("operator")
62-72: The changes look good! Here's a minor suggestion.Using
doc.get(field, None)is a cleaner way to retrieve optional fields. As a small nitpick, you can remove theNonedefault value in all these cases sincegetalready returnsNoneif the key is missing.Apply this diff:
-self.delegation_target = doc.get("delegation_target", None) +self.delegation_target = doc.get("delegation_target") -self.started_at = doc.get("started_at", None) +self.started_at = doc.get("started_at") -self.completed_at = doc.get("completed_at", None) +self.completed_at = doc.get("completed_at") -self.failed_at = doc.get("failed_at", None) +self.failed_at = doc.get("failed_at") -self.pending_at = doc.get("pending_at", None) +self.pending_at = doc.get("pending_at") -self.pinned = doc.get("pinned", None) +self.pinned = doc.get("pinned") -self.dataset_id = doc.get("dataset_id", None) +self.dataset_id = doc.get("dataset_id") -self.run_link = doc.get("run_link", None) +self.run_link = doc.get("run_link") -self.metadata = doc.get("metadata", None) +self.metadata = doc.get("metadata") -self.label = doc.get("label", None) +self.label = doc.get("label") -self.updated_at = doc.get("updated_at", None) +self.updated_at = doc.get("updated_at")Tools
Ruff
62-62: Use
doc.get("delegation_target")instead ofdoc.get("delegation_target", None)Replace
doc.get("delegation_target", None)withdoc.get("delegation_target")(SIM910)
63-63: Use
doc.get("started_at")instead ofdoc.get("started_at", None)Replace
doc.get("started_at", None)withdoc.get("started_at")(SIM910)
64-64: Use
doc.get("completed_at")instead ofdoc.get("completed_at", None)Replace
doc.get("completed_at", None)withdoc.get("completed_at")(SIM910)
65-65: Use
doc.get("failed_at")instead ofdoc.get("failed_at", None)Replace
doc.get("failed_at", None)withdoc.get("failed_at")(SIM910)
66-66: Use
doc.get("pending_at")instead ofdoc.get("pending_at", None)Replace
doc.get("pending_at", None)withdoc.get("pending_at")(SIM910)
67-67: Use
doc.get("pinned")instead ofdoc.get("pinned", None)Replace
doc.get("pinned", None)withdoc.get("pinned")(SIM910)
68-68: Use
doc.get("dataset_id")instead ofdoc.get("dataset_id", None)Replace
doc.get("dataset_id", None)withdoc.get("dataset_id")(SIM910)
69-69: Use
doc.get("run_link")instead ofdoc.get("run_link", None)Replace
doc.get("run_link", None)withdoc.get("run_link")(SIM910)
70-70: Use
doc.get("metadata")instead ofdoc.get("metadata", None)Replace
doc.get("metadata", None)withdoc.get("metadata")(SIM910)
71-71: Use
doc.get("label")instead ofdoc.get("label", None)Replace
doc.get("label", None)withdoc.get("label")(SIM910)
72-72: Use
doc.get("updated_at")instead ofdoc.get("updated_at", None)Replace
doc.get("updated_at", None)withdoc.get("updated_at")(SIM910)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- fiftyone/factory/repos/delegated_operation_doc.py (1 hunks)
- tests/unittests/delegated_operators_tests.py (3 hunks)
Additional context used
Ruff
fiftyone/factory/repos/delegated_operation_doc.py
62-62: Use
doc.get("delegation_target")instead ofdoc.get("delegation_target", None)Replace
doc.get("delegation_target", None)withdoc.get("delegation_target")(SIM910)
63-63: Use
doc.get("started_at")instead ofdoc.get("started_at", None)Replace
doc.get("started_at", None)withdoc.get("started_at")(SIM910)
64-64: Use
doc.get("completed_at")instead ofdoc.get("completed_at", None)Replace
doc.get("completed_at", None)withdoc.get("completed_at")(SIM910)
65-65: Use
doc.get("failed_at")instead ofdoc.get("failed_at", None)Replace
doc.get("failed_at", None)withdoc.get("failed_at")(SIM910)
66-66: Use
doc.get("pending_at")instead ofdoc.get("pending_at", None)Replace
doc.get("pending_at", None)withdoc.get("pending_at")(SIM910)
67-67: Use
doc.get("pinned")instead ofdoc.get("pinned", None)Replace
doc.get("pinned", None)withdoc.get("pinned")(SIM910)
68-68: Use
doc.get("dataset_id")instead ofdoc.get("dataset_id", None)Replace
doc.get("dataset_id", None)withdoc.get("dataset_id")(SIM910)
69-69: Use
doc.get("run_link")instead ofdoc.get("run_link", None)Replace
doc.get("run_link", None)withdoc.get("run_link")(SIM910)
70-70: Use
doc.get("metadata")instead ofdoc.get("metadata", None)Replace
doc.get("metadata", None)withdoc.get("metadata")(SIM910)
71-71: Use
doc.get("label")instead ofdoc.get("label", None)Replace
doc.get("label", None)withdoc.get("label")(SIM910)
72-72: Use
doc.get("updated_at")instead ofdoc.get("updated_at", None)Replace
doc.get("updated_at", None)withdoc.get("updated_at")(SIM910)
Additional comments not posted (5)
fiftyone/factory/repos/delegated_operation_doc.py (4)
49-49: LGTM!Initializing the new
pending_atattribute toNonein the constructor is a valid approach. It will likely store a timestamp related to pending operations.
58-58: LGTM!Using
doc.get("queued_at")is a cleaner way to retrieve the value. The change looks good.
59-59: Looks good!Using
doc.get("run_state")is a cleaner way to retrieve the value. The change is valid.
74-76: LGTM!Setting the internal fields
idand_docafter retrieving the optional fields is a reasonable order. The change looks good.tests/unittests/delegated_operators_tests.py (1)
Line range hint
229-317: LGTM! The test coverage has been improved.The changes in the
test_list_operationsfunction enhance the test coverage by:
- Verifying the behavior of listing operations in different states (queued, running, pending) using dedicated service methods.
- Updating the assertions to ensure the expected behavior is maintained.
- Renaming the test to accurately reflect the expanded scope.
These improvements contribute to a more comprehensive test suite.
|
Per offline discussion, |
CamronStaley
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (8)
fiftyone/factory/repos/delegated_operation_doc.py (8)
62-62: Simplify the retrieval of thedelegation_targetfield.The static analysis hint suggests using
doc.get("delegation_target")instead ofdoc.get("delegation_target", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.delegation_target = doc.get("delegation_target", None) +self.delegation_target = doc.get("delegation_target")Tools
Ruff
62-62: Use
doc.get("delegation_target")instead ofdoc.get("delegation_target", None)Replace
doc.get("delegation_target", None)withdoc.get("delegation_target")(SIM910)
63-63: Simplify the retrieval of thestarted_atfield.The static analysis hint suggests using
doc.get("started_at")instead ofdoc.get("started_at", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.started_at = doc.get("started_at", None) +self.started_at = doc.get("started_at")Tools
Ruff
63-63: Use
doc.get("started_at")instead ofdoc.get("started_at", None)Replace
doc.get("started_at", None)withdoc.get("started_at")(SIM910)
64-64: Simplify the retrieval of thecompleted_atfield.The static analysis hint suggests using
doc.get("completed_at")instead ofdoc.get("completed_at", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.completed_at = doc.get("completed_at", None) +self.completed_at = doc.get("completed_at")Tools
Ruff
64-64: Use
doc.get("completed_at")instead ofdoc.get("completed_at", None)Replace
doc.get("completed_at", None)withdoc.get("completed_at")(SIM910)
65-65: Simplify the retrieval of thefailed_atfield.The static analysis hint suggests using
doc.get("failed_at")instead ofdoc.get("failed_at", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.failed_at = doc.get("failed_at", None) +self.failed_at = doc.get("failed_at")Tools
Ruff
65-65: Use
doc.get("failed_at")instead ofdoc.get("failed_at", None)Replace
doc.get("failed_at", None)withdoc.get("failed_at")(SIM910)
66-66: Simplify the retrieval of thescheduled_atfield.The static analysis hint suggests using
doc.get("scheduled_at")instead ofdoc.get("scheduled_at", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.scheduled_at = doc.get("scheduled_at", None) +self.scheduled_at = doc.get("scheduled_at")Tools
Ruff
66-66: Use
doc.get("scheduled_at")instead ofdoc.get("scheduled_at", None)Replace
doc.get("scheduled_at", None)withdoc.get("scheduled_at")(SIM910)
67-67: Simplify the retrieval of thepinnedfield.The static analysis hint suggests using
doc.get("pinned")instead ofdoc.get("pinned", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.pinned = doc.get("pinned", None) +self.pinned = doc.get("pinned")Tools
Ruff
67-67: Use
doc.get("pinned")instead ofdoc.get("pinned", None)Replace
doc.get("pinned", None)withdoc.get("pinned")(SIM910)
68-68: Simplify the retrieval of thedataset_idfield.The static analysis hint suggests using
doc.get("dataset_id")instead ofdoc.get("dataset_id", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.dataset_id = doc.get("dataset_id", None) +self.dataset_id = doc.get("dataset_id")Tools
Ruff
68-68: Use
doc.get("dataset_id")instead ofdoc.get("dataset_id", None)Replace
doc.get("dataset_id", None)withdoc.get("dataset_id")(SIM910)
69-69: Simplify the retrieval of therun_linkfield.The static analysis hint suggests using
doc.get("run_link")instead ofdoc.get("run_link", None). The default value ofNoneis redundant, as thegetmethod already returnsNoneif the key is not found in the dictionary.Apply this diff to simplify the code:
-self.run_link = doc.get("run_link", None) +self.run_link = doc.get("run_link")Tools
Ruff
69-69: Use
doc.get("run_link")instead ofdoc.get("run_link", None)Replace
doc.get("run_link", None)withdoc.get("run_link")(SIM910)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (8)
- docs/source/cli/index.rst (1 hunks)
- fiftyone/core/cli.py (2 hunks)
- fiftyone/factory/init.py (1 hunks)
- fiftyone/factory/repos/delegated_operation.py (3 hunks)
- fiftyone/factory/repos/delegated_operation_doc.py (1 hunks)
- fiftyone/operators/delegated.py (2 hunks)
- fiftyone/operators/executor.py (1 hunks)
- tests/unittests/delegated_operators_tests.py (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- docs/source/cli/index.rst
- fiftyone/core/cli.py
- fiftyone/factory/init.py
- fiftyone/factory/repos/delegated_operation.py
- fiftyone/operators/executor.py
- tests/unittests/delegated_operators_tests.py
Additional context used
Ruff
fiftyone/factory/repos/delegated_operation_doc.py
62-62: Use
doc.get("delegation_target")instead ofdoc.get("delegation_target", None)Replace
doc.get("delegation_target", None)withdoc.get("delegation_target")(SIM910)
63-63: Use
doc.get("started_at")instead ofdoc.get("started_at", None)Replace
doc.get("started_at", None)withdoc.get("started_at")(SIM910)
64-64: Use
doc.get("completed_at")instead ofdoc.get("completed_at", None)Replace
doc.get("completed_at", None)withdoc.get("completed_at")(SIM910)
65-65: Use
doc.get("failed_at")instead ofdoc.get("failed_at", None)Replace
doc.get("failed_at", None)withdoc.get("failed_at")(SIM910)
66-66: Use
doc.get("scheduled_at")instead ofdoc.get("scheduled_at", None)Replace
doc.get("scheduled_at", None)withdoc.get("scheduled_at")(SIM910)
67-67: Use
doc.get("pinned")instead ofdoc.get("pinned", None)Replace
doc.get("pinned", None)withdoc.get("pinned")(SIM910)
68-68: Use
doc.get("dataset_id")instead ofdoc.get("dataset_id", None)Replace
doc.get("dataset_id", None)withdoc.get("dataset_id")(SIM910)
69-69: Use
doc.get("run_link")instead ofdoc.get("run_link", None)Replace
doc.get("run_link", None)withdoc.get("run_link")(SIM910)
70-70: Use
doc.get("metadata")instead ofdoc.get("metadata", None)Replace
doc.get("metadata", None)withdoc.get("metadata")(SIM910)
71-71: Use
doc.get("label")instead ofdoc.get("label", None)Replace
doc.get("label", None)withdoc.get("label")(SIM910)
72-72: Use
doc.get("updated_at")instead ofdoc.get("updated_at", None)Replace
doc.get("updated_at", None)withdoc.get("updated_at")(SIM910)
Additional comments not posted (7)
fiftyone/factory/repos/delegated_operation_doc.py (4)
49-49: LGTM!The initialization of
self.scheduled_attoNonein the constructor is consistent with the initialization of other optional fields.
57-57: LGTM!The use of the
getmethod for retrieving theoperatorfield from thedocdictionary is consistent with the suggestion from the past review comments.
58-58: LGTM!The use of the
getmethod for retrieving thequeued_atfield from thedocdictionary is consistent with the suggestion from the past review comments.
59-59: LGTM!The use of the
getmethod for retrieving therun_statefield from thedocdictionary is consistent with the suggestion from the past review comments.fiftyone/operators/delegated.py (3)
104-113: LGTM!The
set_scheduledmethod is implemented correctly. It updates the run state of a delegated operation toExecutionRunState.SCHEDULEDusing the provideddoc_id.
249-261: LGTM!The
get_scheduled_operationsmethod is implemented correctly. It retrieves all scheduled delegated operations from the repository, with optional filtering byoperatoranddataset_name.
263-275: LGTM!The
get_running_operationsmethod is implemented correctly. It retrieves all running delegated operations from the repository, with optional filtering byoperatoranddataset_name.
| self.operator = doc.get("operator") | ||
| self.queued_at = doc.get("queued_at") | ||
| self.run_state = doc.get("run_state") |
There was a problem hiding this comment.
Don't really need to use get() for these as they're required to be set. It just pushes the errors to later
| self.pinned = doc["pinned"] if "pinned" in doc else None | ||
| self.dataset_id = doc["dataset_id"] if "dataset_id" in doc else None | ||
| self.run_link = doc["run_link"] if "run_link" in doc else None | ||
| self.delegation_target = doc.get("delegation_target", None) |
There was a problem hiding this comment.
technically None is already the default so not necessary, but it's fine to leave it
What changes are proposed in this pull request?
Add scheduled state for delegated operations collection
How is this patch tested? If it is not, please explain why.
tested in fiftyone celery poc
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
Documentation