-
Notifications
You must be signed in to change notification settings - Fork 67
Add workflow management #1975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add workflow management #1975
Conversation
e6267da
to
fb437f0
Compare
@@ -59,6 +59,7 @@ | |||
ProjectOverview, | |||
ProjectOverviewDetailed, | |||
) | |||
from labelbox.schema.workflow import ProjectWorkflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local imports below should not be necessary if provided here.
In general, it would be great to not have any local imports, if it's to avoid a circular dependency perhaps we can see if it's easy to resolve and perhaps mark as a TODO if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed a few and add the TODOs after several attempts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this into nodes.py
as it's the base class for all nodes in the graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code refactored. I keep this one isolated.
"ReviewTime": "review_time", | ||
"NlSearch": "natural_language", | ||
"LabelFeedback": "label_feedback", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to create this mapping from ALIAS_TO_BACKEND_KEY
to avoid duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code refactored
} | ||
|
||
|
||
def normalize_filter_rule(rule: Dict[str, Any]) -> Dict[str, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I might have considered using enums more generally (as opposed to strings) to minimize the need for translating between formats - enums are also easier to use from a readability / usability standpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation refactored
label: str = Field(default="Initial labeling task", frozen=True) | ||
filter_logic: Literal["and", "or"] = Field( | ||
default="and", alias="filterLogic" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this only applies to a logical node, perhaps it should be defined as frozen on the base type (or ideally only defined on the logical node).
return self._edge_factory | ||
|
||
@property | ||
def WorkflowEdge(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer create_edge
, also it's a bit unorthodox to have a property return a callable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code refactored
return node | ||
|
||
# Node creation methods | ||
def InitialLabelingNode(self, **kwargs) -> InitialLabelingNode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I prefer create_initial_labeling_node
or verb-based method names - this is a bit unorthodox but it's fine if this a convention used by the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code refactored
# For any other type, return None to avoid serialization errors | ||
return None | ||
|
||
def reset_config(self) -> "ProjectWorkflow": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We might want to consider creating the default nodes as part of this process - I'll leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do it to ensure that user could customize their initial nodes even if, currently, only the initial labeling node can receive instructions. The current approach makes it easier if we decide to add more customization in the future.
723b059
to
62da8c5
Compare
62da8c5
to
4c95356
Compare
4c95356
to
bfa1df9
Compare
Description
This PR adds workflow management to the SDK.
Note: the verb "create" is used abusively since one doesn't create a workflow but updates it either by removing all nodes first (
reset_config()
) or by changing the existing configuration.project.get_workflow()
andproject.clone_workflow_from()
will raise a warning:Results:

Fixes # (issue)
Type of change
Please delete options that are not relevant.
All Submissions
New Feature Submissions
Changes to Core Features