-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add new endpoints for workflows and prepare for future deprecation #336
Add new endpoints for workflows and prepare for future deprecation #336
Conversation
…precation of new parameter later on
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.
Only got partway through reviewing but have to go into meetings for a bit. Will finish later but wanted to get these comments out there.
description="Parameter to decide if Active Learning data sampling is disabled for the model", | ||
examples=[True, "$inputs.disable_active_learning"], | ||
) | ||
active_learning_target_dataset: Union[ |
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.
Would be useful to add an array of tag strings that get added to the uploaded images.
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.
would be, but this is also possible in AL config that can be declared in-place. We can add this feature, but I would put that into to-do for next iteration
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.
this is also possible in AL config that can be declared in-place
Can you clarify? I don't think I understand what this is
I would put that into to-do for next iteration
Sounds good
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.
this is also possible in AL config that can be declared in-place
-> you can request specific tags to be added via config of AL that can be defined either at project level or in-place
@@ -432,31 +511,44 @@ def validate_field_binding(self, field_name: str, value: Any) -> None: | |||
|
|||
|
|||
class InstanceSegmentationModel(ObjectDetectionModel): |
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.
Is there an easy way to know from the manifest whether something is a subclass of BaseModel? I think right now I just check if the name ends in _model
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.
do you mean BaseModel
or RoboflowModel
.
If the former - each manifest class must be derived after pydantic.BaseModel
If the latter - no way now - I feel like additional metadata about steps can be held in
model_config = ConfigDict(
json_schema_extra={
"description": "This block represents inference from Roboflow multi-class classification model.",
"docs": "https://inference.roboflow.com/workflows/classify_objects", <- additional keys here
}
)
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.
Is that in the manifest? I don't see BaseModel
or base_model
in there anywhere.
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.
no it is not, that's just required for pydantic to work
@@ -676,15 +855,26 @@ def validate_field_binding(self, field_name: str, value: Any) -> None: | |||
|
|||
|
|||
class BarcodeDetection(BaseModel, StepInterface): |
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.
Should we rename this BarcodeModel since it inherits from BaseModel? (At the least should be BarcodeDetector vs Detection to match the part of speech of other blocks)
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.
that's pydantic.BaseModel
.
BarcodeDetector
is fine, we would also need to change QRCodeDetection
in the same way
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.
ok, changed.
Also made the change non-breaking by double-aliasing step type - we will sun-set old ones in some time
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.
Also wonder if eg YoloWorld should be called YoloWorldModel
I am branching out from this ref. point to re-structure the workflows steps - I propose to only fix issues in this PR, additional feature requests I will implement later |
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.
Read through the rest & looks good!
@@ -432,31 +511,44 @@ def validate_field_binding(self, field_name: str, value: Any) -> None: | |||
|
|||
|
|||
class InstanceSegmentationModel(ObjectDetectionModel): |
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.
Is that in the manifest? I don't see BaseModel
or base_model
in there anywhere.
Description
There were couple of changes in this PR:
Changes in workflows endpoints
Adjusted sdk client to changes
Maintained backward compatibility of endpoints
Provided self-descriptive capabilities for
workflows
blocks and endpoints to describe all loaded stepsProvided endpoint to validate
workflows
JSON definitionsImproved significantly
workflows
syntax validation by establishing typedefs with constraint checks inpydantic
OLD:
NEW:
Thanks to that: ➖ 900 lines of code
Registered workflows in
InferencePipeline
Fixed couple of bugs
workflows
at hosted platformType of change
Please delete options that are not relevant.
How has this change been tested, please provide a testcase or example of how you tested the change?
workflows
- which means that we shall not expect breaking change in thatAny specific deployment considerations
workflows
contractDocs
workflows
,InferencePipeline
inference_sdk