Skip to content
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

Support tasks creating filters both in legacy or new way #2172

Closed
tcompa opened this issue Jan 8, 2025 · 4 comments
Closed

Support tasks creating filters both in legacy or new way #2172

tcompa opened this issue Jan 8, 2025 · 4 comments
Labels

Comments

@tcompa
Copy link
Collaborator

tcompa commented Jan 8, 2025

Current server/task interface includes:

class TaskOutput(BaseModel, extra=Extra.forbid):
    filters: Filters = Field(default_factory=Filters)

As of #2168, in principle this should become

class TaskOutput(BaseModel, extra=Extra.forbid):
    type_filters: dict[str, bool] = Field(default_factory=dict)

but doing so would break the existing server/task interface.

We have two options:

Option 1 (backwards compatible)

We accept both the previous and upcoming task outputs, and convert them all to the upcoming version

class TaskOutput(BaseModel, extra=Extra.forbid):
    type_filters: dict[str, bool] = Field(default_factory=dict)
    filters: dict[str, dict[str, Any]] = Field(default_factory=Filters)

    # if only type_filters is set -> OK
    # if both type_filters and filters are set -> Fail
    # if only filters is set, and it includes the "attributes" key -> Fail
    # if only filters is set, and it only includes the "types" key -> Set type_filters to filters["types"]

Option 2 (breaking)

We simply fail for any task that sets filters, that is, we only support the upcoming version.

class TaskOutput(BaseModel, extra=Extra.forbid):
    type_filters: dict[str, bool] = Field(default_factory=dict)
@tcompa
Copy link
Collaborator Author

tcompa commented Jan 8, 2025

Option 1 is obviously safer, but if we think that nobody ever used filters in a task output (or we accept the trade-off of telling them how to change their tasks) then option 2 is simpler.

cc @jluethi

@tcompa tcompa mentioned this issue Jan 8, 2025
3 tasks
@jluethi
Copy link
Collaborator

jluethi commented Jan 8, 2025

Hmm, I think some type filters are used in tasks. For example here:
https://github.com/fractal-analytics-platform/fractal-helper-tasks/blob/19060daca2ad8635ec637e7b94fc89fdfa6e9360/src/fractal_helper_tasks/drop_t_dimension.py#L87

And here:
https://github.com/fractal-analytics-platform/fractal-helper-tasks/blob/19060daca2ad8635ec637e7b94fc89fdfa6e9360/src/fractal_helper_tasks/rechunk_zarr.py#L134

We also document this feature here: https://fractal-analytics-platform.github.io/tasks_spec/#output-api

Thus, I'd say we should use Option 1. And then update both the documentation as well as the known examples to move to the new, simpler system

@tcompa tcompa changed the title To decide: should we support the "old" way for tasks to set filters? Support tasks creating filters both as legacy or new way Jan 15, 2025
@tcompa tcompa changed the title Support tasks creating filters both as legacy or new way Support tasks creating filters both in legacy or new way Jan 15, 2025
@tcompa
Copy link
Collaborator Author

tcompa commented Jan 20, 2025

This is made obsolete by #2188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

2 participants