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

Delete a WorkflowTask argument (when not using JSON Schemas) #197

Closed
tcompa opened this issue Jun 21, 2023 · 7 comments · Fixed by #481
Closed

Delete a WorkflowTask argument (when not using JSON Schemas) #197

tcompa opened this issue Jun 21, 2023 · 7 comments · Fixed by #481

Comments

@tcompa
Copy link
Collaborator

tcompa commented Jun 21, 2023

NOTE: this issue applies to the "legacy" free-arguments component, not to the one based on JSON Schemas.

See fractal-analytics-platform/fractal-server#629 (comment):

As of #758, fractal-server now allows deleting an argument via the PATCH-WorkflowTask endpoint.

If the current WorkflowTask.args is {"a": 1, "b": 2}, then sending a PATCH request with args={"a": 3} will lead to a new WorkflowTask.args = {"a":3} (without b).

This behavior is available for all arguments that do not have a default value in the task args_schema (or for all arguments of tasks without args_schema). If a task has an args_schema, and b has a default, then with the same procedure as above we'll end up with args = {"a": 3, "b": DEFAULT_OF_B}.

It is not possible to fully remove an argument if it has a default value in the schema, and we are not planning to introduce this feature for the moment.

This means that as of fractal-server 1.3.0a9 we can implement a feature to remove an argument.

@tcompa tcompa added fractal-server-1.3 feature Feature description issues and removed fractal-server-1.3 labels Jun 21, 2023
@tcompa tcompa added fractal-server-v2 and removed feature Feature description issues labels Apr 10, 2024
@tcompa
Copy link
Collaborator Author

tcompa commented Apr 10, 2024

(context: fractal-analytics-platform/fractal-server#1369)

In V2, the args_non_parallel property in a request body for PATCH /project/{project_id}/workflow/{workflow_id}/wftask/{workflow_task_id}/ fully replaces the original database value (provided the task doesn't have any JSON Schema). The same holds for args_parallel.

We should likely update the non-JSON-schema-based WorkflowTask arguments forms. The web-client form handler is responsible of preparing a payload with the entire expected value of e.g. args_parallel, rather than just the modified parts.

Example of expected behavior:

  1. We have a parallel task, with no JSON Schemas
  2. The current value of args_parallel is {"a": "b"}
  3. In the form, we remove this key-value pair, and create a new one {"c": "d"}
  4. We save, thus triggering the PATCH call with the request body including {"args_parallel": {"c": "d"}}

Side-comment: it looks to me that this is also the V1 behavior, server-side. To be verified.

@tcompa
Copy link
Collaborator Author

tcompa commented Apr 10, 2024

Side-comment: it looks to me that this is also the V1 behavior, server-side. To be verified.

I confirm this is the backend behavior since version 1.3.0 (ref fractal-analytics-platform/fractal-server#758).
Therefore this only requires a quick review, in fractal-web. Likely it was already fixed, and this issue was left open by mistake.

@tcompa
Copy link
Collaborator Author

tcompa commented May 2, 2024

(context: fractal-analytics-platform/fractal-server#1369)

In V2, the args_non_parallel property in a request body for PATCH /project/{project_id}/workflow/{workflow_id}/wftask/{workflow_task_id}/ fully replaces the original database value (provided the task doesn't have any JSON Schema). The same holds for args_parallel.

We should likely update the non-JSON-schema-based WorkflowTask arguments forms. The web-client form handler is responsible of preparing a payload with the entire expected value of e.g. args_parallel, rather than just the modified parts.

Example of expected behavior:

1. We have a parallel task, with no JSON Schemas

2. The current value of `args_parallel` is `{"a": "b"}`

3. In the form, we remove this key-value pair, and create a new one `{"c": "d"}`

4. We save, thus triggering the PATCH call with the request body including `{"args_parallel": {"c": "d"}}`

Side-comment: it looks to me that this is also the V1 behavior, server-side. To be verified.

This expected behavior is most likely already in-place.
Let's add a test for it, if missing.

@zonia3000
Copy link
Collaborator

This expected behavior is most likely already in-place. Let's add a test for it, if missing.

This is not working yet, because the delete button is disabled when an argument has already been saved, and it looks that it was done because in the UI you can edit only one item at a time. I've always find that quite weird, so I'm going to refactor this a bit to have a more flexible editing. Moreover it seems that adding an array arguments doesn't work, so also more testing on this part is needed.

@tcompa
Copy link
Collaborator Author

tcompa commented May 3, 2024

To review here: are we correctly handling parallel/nonparallel/compound tasks?

@zonia3000
Copy link
Collaborator

In the new form it is possible to edit all the fields at the same time, as happens for type and attributes:

image

I put inside the accordion header the text field and the delete button for the object and array keys, so that one can edit or delete also array and object properties.

The same component was used also for editing meta properties, so this is how appears now the meta properties tab:

image

Question: we renamed "Args non parallel" to "Initialisation Parameters" and "Args parallel" to "Compute Parameters". Shouldn't we do the same for the meta tab? Something like "Initialisation Meta" and "Compute Meta"?

@jluethi
Copy link
Collaborator

jluethi commented May 7, 2024

Something like "Initialisation Meta" and "Compute Meta"?

Agreed!

In the new form it is possible to edit all the fields at the same time

That's great! It makes me think of this new, broader issue here: #482

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

Successfully merging a pull request may close this issue.

3 participants