Add request param overrides for pipeline operators#7277
Add request param overrides for pipeline operators#7277CamronStaley wants to merge 2 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA documentation section was added to explain how pipeline stages can override request parameters using Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
swheaton
left a comment
There was a problem hiding this comment.
lgtm but see what @AdonaiVera wants to do with it
(We have a docs addition but don't want to target today's release which is underway already)
AdonaiVera
left a comment
There was a problem hiding this comment.
I just committed some small doc style improvements @CamronStaley, this PR is ready!
Feel free to merge whenever you think it makes sense to have it in production. Once it’s merged, I can add a tag to deploy the docs.
There was a problem hiding this comment.
@CamronStaley @swheaton I might have missed the boat on providing feedback on the functional implementation of this feature, but please note I have a small FR on the view parameter 😄
| request_params_overrides={"view_name": "val_split"}, | ||
| ) | ||
|
|
||
| # Stage 3 runs against the full dataset (no view applied) |
There was a problem hiding this comment.
Can you add a case where where a view is provided?
I imagine a typical case would be something like view=ctx.view[i:j], ie manual batching.
|
|
||
| - `view` — a list of view stages to apply | ||
| - `view_name` — the name of a saved view to use | ||
| - `filters` — a dictionary of filters to apply |
There was a problem hiding this comment.
filters is not an officially documented thing. How would users figure out how to successfully use this?
There was a problem hiding this comment.
They wouldn't. We can remove this example from the documentation
| shadow the corresponding value from the top-level request for that stage only; | ||
| all other stages are unaffected. | ||
|
|
||
| Overridable parameters include: |
There was a problem hiding this comment.
Can dataset or dataset_name be overridden? That would be powerful, but may be dubious too.
There was a problem hiding this comment.
I was thinking about disallowing that but i guess we never did it. Probably should have an allow or block list.
Allowed and potentially useful
- view_name
- view (*****)
- selected
- current_sample
- active_fields
- group_slice
- num_distributed_tasks
Allowed (not exactly useful but not harmful either)
- filters
- extended
- workspace_name
- spaces
- selected_labels
- extended_selection
- query_performance
Disallowed
- dataset_id
- dataset_name
- delegated
- request_delegation
- results
Unsure
- delegation target - I'm not sure if this would cause downstream issues. Thinking not, but we may want a more official way to change delegation targets rather than the pipeline operator doing it under the hood
There was a problem hiding this comment.
Changing delegation target would be useful in the sense that certain stages may need to use GPU.
Certainly it would be an anti-pattern for pipeline operators to hard-code the name of a delegation target in them, however.
There was a problem hiding this comment.
yeah this would be a cool feature whenever we have more information about orcs in the collection. Would be cool if they could say something like "delegate_criteria: must have gpu, X storage, blah" and then we could select an appropriate orc on their behalf.
|
|
||
| Overridable parameters include: | ||
|
|
||
| - `view` — a list of view stages to apply |
There was a problem hiding this comment.
Can we please support view being provided as a DatasetView object? Which would be serialized into view stages internally by our interface.
I think our public interfaces should always work with FO's main types, ie support DatasetView, not require lists of ViewStages and certainly not requiring lists of serialized view stage dicts.
(it is fwm if the view parameter supports the latter two possibilities as undocumented syntaxes though)
There was a problem hiding this comment.
agreed currently you have to serialize it (we don't do that on behalf of the user) which isn't intuitive. I can open a PR to fix that mb
|
Going to leave this PR open / undocumented to users until I can address Bmoore's comments in the next release. Current plan is:
|
|
holding off docs until code matches what's expected from above |
🔗 Related Issues
📋 What changes are proposed in this pull request?
Update docs for request param overrides.
🧪 How is this patch tested? If it is not, please explain why.
n/a
📝 Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
Added instructions to docs for how to use request param overrides in pipeline operators.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
request_params_overrides.