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

Nested default parameters not shown in workflow page #537

Closed
jluethi opened this issue Jul 24, 2024 · 6 comments
Closed

Nested default parameters not shown in workflow page #537

jluethi opened this issue Jul 24, 2024 · 6 comments
Labels
bug Something isn't working JSON Schemas Editing of WorkflowTask arguments based on JSON Schemas

Comments

@jluethi
Copy link
Collaborator

jluethi commented Jul 24, 2024

For different task with Pydantic models (e.g. the Cellpose task with the Advanced Cellpose Model Params or the Normalize model within the channel) don't show the defaults of those models correctly in the workflow page, while it's displayed correctly in the sandbox page.

Workflow page with Fractal web 1.3.2
Screenshot 2024-07-24 at 17 46 34

Workflow page with Fractal web 1.3.0
Screenshot 2024-07-24 at 17 46 29

Sandbox page
Screenshot 2024-07-24 at 17 46 43

I think the issue is with models that are specified as a reference:

"advanced_cellpose_model_params": {
            "$ref": "#/definitions/CellposeModelParams",
            "title": "Advanced_Cellpose_Model_Params",
            "description": "Advanced Cellpose model parameters that are passed to the Cellpose `model.eval` method."
          },

Originally reported by @adrtsc

@jluethi jluethi added the bug Something isn't working label Jul 24, 2024
@jluethi
Copy link
Collaborator Author

jluethi commented Jul 24, 2024

Schema that reproduces the issue:
FRACTAL_MANIFEST.json

Both for the Advanced Cellpose Model Params & the Normalize model within the channel in the Cellpose Segmentation task

@tcompa
Copy link
Collaborator

tcompa commented Jul 24, 2024

Quick feedback: are we looking at the same observations as in the fractal-analytics-platform/fractal-tasks-core#738 discussion?

I'll need to double check, but our first understanding is that the backend does not populate default values for non-top-level arguments. This means we would need to modify the task call signature (similar to what we did in the PR mentioned above) so that the default is with the top-level arguments and not only in the nested ones.

More on this later.

@tcompa
Copy link
Collaborator

tcompa commented Jul 25, 2024

In #538, I laid out our current review of this issue, and the different behavior between sandbox and fractal-web is understood.

I'd say that the current issue is not a regression with recent fractal-web versions, but it's rather due to recent changes in the task call signatures which we did not review carefully enough (interesting side question: how should we test that a new schema produces the expected UI? TBD).

I think a change to fractal-web will be necessary (ref #539), but it's not something that can be rushed through. My suggestion for a quick fix is to review the task call-signature and look for a workaround that works as expected. I will start looking at some options over there.

@tcompa tcompa added the JSON Schemas Editing of WorkflowTask arguments based on JSON Schemas label Jul 25, 2024
@jluethi
Copy link
Collaborator Author

jluethi commented Jul 25, 2024

interesting side question: how should we test that a new schema produces the expected UI

I'd say the sandbox is how task developers will test whether a given manifest will have the desired UI behavior. And eventually having it on the actual instance. But this highlights the importance of having the sandbox fully reflect how the front-end will work with its backend interaction more. That may mean simplifying the backend (as in fractal-analytics-platform/fractal-server#1686) or adding some complexity to the sandbox where well-understood and reasonable.

The design goal to work towards here being that task developers can fully verify the eventual behavior by using the sandbox. The fact that there is complexity in the Pydantic + json manifests + backend server with defaults should not be a surprise in retrospective :) Let's use this to reflect on which complexity in these systems is actually warranted (=> will be also added to the sandbox) and which complexity can be refactored away.

@tcompa
Copy link
Collaborator

tcompa commented Jul 25, 2024

That may mean simplifying the backend (as in fractal-analytics-platform/fractal-server#1686) or adding some complexity to the sandbox where well-understood and reasonable.

This was implemented in the former way, by simplifying the backend.
Refs:

When using fractal-server 2.3.6, upon adding a cellpose task we observe the expected behavior:
image
image

@tcompa tcompa closed this as completed Jul 25, 2024
@tcompa
Copy link
Collaborator

tcompa commented Jul 25, 2024

Thanks a lot @adrtsc for reporting this! It led to very useful discussions and updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working JSON Schemas Editing of WorkflowTask arguments based on JSON Schemas
Projects
Development

No branches or pull requests

2 participants