-
Notifications
You must be signed in to change notification settings - Fork 11
Use SliceBuilder endpoint #365
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
Conversation
gatli
left a comment
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.
LGTM 👍
I'd just add docstrings to explain the dataclasses
| def create_slice_builder_payload( | ||
| name: str, | ||
| sample_size: int, | ||
| sample_method: Union[str, "SliceBuilderMethods"], | ||
| filters: Optional["SliceBuilderFilters"], | ||
| ): | ||
| # enum or string | ||
| sampleMethod = ( | ||
| sample_method.value | ||
| if isinstance(sample_method, SliceBuilderMethods) | ||
| else sample_method | ||
| ) | ||
|
|
||
| filter_payload: Dict[str, Union[str, dict]] = {} | ||
| if filters is not None: | ||
| if filters.slice_id is not None: | ||
| filter_payload["sliceId"] = filters.slice_id | ||
| if filters.autotag is not None: | ||
| filter_payload["autotag"] = { | ||
| "autotagId": filters.autotag.autotag_id, | ||
| "range": filters.autotag.score_range, | ||
| } | ||
|
|
||
| return { | ||
| "name": name, | ||
| "sampleSize": sample_size, | ||
| "sampleMethod": sampleMethod, | ||
| "filters": filter_payload, | ||
| } |
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'm not a fan of using helper methods for payload building. I'd rather just use pydantic off the bat and serialize that. Using validators for sanitizing gives them a "home" that is guaranteed to be called for the payload.
But yeah, don't worry about refactoring this. If it works we can merge it.
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've forgotten about pydantic. But its a good callout, would be better to have everything in one central place.
If i revisit this, i will migrate it to pydantic
Co-authored-by: Gunnar Atli Thoroddsen <[email protected]>
| @pytest.mark.skip( | ||
| reason="Skip Temporarily - Need to find issue with customObjectIndexingJobId" | ||
| ) | ||
| def test_box_gt_upload_embedding(CLIENT, dataset): |
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.
@gatli I am skipping these two tests for now, to unblock CI.
They are consistently failing with customObjectIndexingJobId not being in the response.
ie: https://app.circleci.com/pipelines/github/scaleapi/nucleus-python-client/2311/workflows/8c84e971-e6c9-47d3-889a-c7616871f433/jobs/4800/parallel-runs/2
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.
Use the new endpoint
build_sliceto create a slice with Smart Sample.Works for the backend in the PR: https://github.com/scaleapi/scaleapi/pull/48465
Examples: