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

Default WorkflowTask arguments #127

Closed
tcompa opened this issue Apr 21, 2023 · 3 comments
Closed

Default WorkflowTask arguments #127

tcompa opened this issue Apr 21, 2023 · 3 comments

Comments

@tcompa
Copy link
Collaborator

tcompa commented Apr 21, 2023

Branching from #82, since it is a more specific discussion.

@rkpasia be aware that this discussion touches multiple Fractal components/repositories, even if it is fully driven by the needs of the web client.


To be clarified: what does the user see when they create a WorkflowTask? For sure they should see the N arguments, but what are the defaults?

I think we should move the defaults also into the pydantic schemes and optional parameters should have a default. If no default is provided, it defaults to None => empty box. None needs to be a valid value then for optional arguments, but not for required arguments.. And all defaults are shown.

Let me rephrase my question, to clarify what we had in mind.

When the user adds a WorkflowTask in the web client, the situation is that

  • The WorkflowTask.args field is empty (this could change with Import Task default attributes WorkflowTask attributes fractal-server#592),
  • The WorkflowTask.task.default_args may include some defaults (but not necessarily all of them),
  • The JSON schema (defined in, say, WorkflowTask.task.args_schema) may include some defaults (the ones coming from the pydantic model, since we are moving away from defaults in the python task function). These defaults are not part of the database WorkflowTask.args entry.

I/we have a few considerations

(A) Too many sources? Should we drop Task.default_args?

My first (high-level) point is that we have many sources for the same information (the JSON schema and the default_args field). I'm in favor of dropping the Task.default_args attribute, and moving this information into the Task.args_schema field.
Having both clearly introduces some confusion: they are both named "default", but one of them has to take priority. Which one? And why?

Side effect: when using the CLI client (where the args_schema attribute is ignored), we would not be able to rely on the task default arguments. Or, if we thinks it's crucial, we will have to implement the handling of schemas also in the CLI client (which doesn't seem ideal, but can be done).

(B) What is shown when a WorkflowTask is inserted?

Original question: what does the user see when they create a WorkflowTask?

Concerning pre-filled default values, there are two extreme cases that are both unsatisfactory:

  1. We only show the pre-filled value for argument X if it present in WorkflowTask.args (or in WorkflowTask.task.default_args, as per the current DB structure - but this could change with (A) or with Import Task default attributes WorkflowTask attributes fractal-server#592). This means that JSON-schema defaults are not visible -> not a good option.
  2. We only show the JSON-schema defaults -> this creates a confusing situation, because those values are only known locally in the web client (which parsed the JSON schema and found out that the default for X is 123), but they are not yet part of WorkflowTask.args until we make an API call.

I'll rephrase once more in more precise terms.

  • We insert a certain Task as a WorkflowTask.
  • WorkflowTask.args is empty.
  • Task.default_args is not present (in line with my proposal (A), if accepted)
  • Task.args_schema has the following specification
    • Argument X is an integer with no default.
    • Argument Y is an integer with default 1.
    • Argument Z is an optional integer (note that optional integer, as in Z: typing.Optional[int], has a well-defined meaning, and it corresponds to Z: Union[int, None] = None).
  • What should the user see? This is not well defined, because WorkflowTask.args is empty.

Proposed solution: the JSON schema defaults must be used to pre-fill WorkflowTask.args when we insert it into a workflow (à la fractal-analytics-platform/fractal-server#592). In this way, it's clear what the user would see the first time they insert a Task into a Workflow.

To be checked: while merging Task.default_args into WorkflowTask.args is trivial, we should further explore how to do it with Task.args_schema and WorkflowTask.args.

@jluethi
Copy link
Collaborator

jluethi commented Apr 21, 2023

My first (high-level) point is that we have many sources for the same information (the JSON schema and the default_args field). I'm in favor of dropping the Task.default_args attribute, and moving this information into the Task.args_schema field.
Having both clearly introduces some confusion: they are both named "default", but one of them has to take priority. Which one? And why?

Very good point. It appears to me that the json schemas are the more general version of defaults. Can we make the json schema the new default args? i.e. the server doesn't have a separate default args, but when adding a task to a workflow, the schema is used to set defaults (=> to set the initial values of workflowTask.args, not to populate a separate default_args entry or anything of the sort). Both on the web and on the CLI.
On the CLI, if a user adds a task with their own json arguments, those then overwrite the defaults from the json schema.
=> the only way to provide default args would be to provide a json schema then.

Or is there a reason to have both default_args and json schema?

We only show the JSON-schema defaults -> this creates a confusing situation, because those values are only known locally in the web client (which parsed the JSON schema and found out that the default for X is 123), but they are not yet part of WorkflowTask.args until we make an API call.

Unless we add the json-schema as the workflow args when adding a task?

Proposed solution: the JSON schema defaults must be used to pre-fill WorkflowTask.args when we insert it into a workflow

Sounds good to me!

@tcompa
Copy link
Collaborator Author

tcompa commented Apr 21, 2023

Very good point. It appears to me that the json schemas are the more general version of defaults. Can we make the json schema the new default args? i.e. the server doesn't have a separate default args, but when adding a task to a workflow, the schema is used to set defaults.

Agreed

Or is there a reason to have both default_args and json schema?

I don't see any reason for that.


Both on the web and on the CLI.
On the CLI, if a user adds a task with their own json arguments, those then overwrite the defaults from the json schema.

Let's check if I understand correctly:

What you would be is the current "WorkflowTask.args has priority over WorkflowTask.task.default_args" situation, rephrased as "WorkflowTask.args has priority over WorkflowTask.task.args_schema defaults". The user would insert a WorkflowTask with fractal workflow add-task <workflow_id> <task_id_or_name> --args-file some_args_file.json, and some_args_file.json would look like

{
 "Y": 1231,
}

To make it explicit, I don't think that overriding a default value should ever require the user to prepare a JSON file like

{
  "title": "TaskArguments",
  "type": "object",
  "properties": {
    "Y": {
      "title": "Y",
      "default": 1,
      "type": "integer"
    }
  }
}

We only show the JSON-schema defaults -> this creates a confusing situation, because those values are only known locally in the web client (which parsed the JSON schema and found out that the default for X is 123), but they are not yet part of WorkflowTask.args until we make an API call.

Unless we add the json-schema as the workflow args when adding a task?

Exactly: If we do follow up as described above&below, then this specific issue is fixed.

Proposed solution: the JSON schema defaults must be used to pre-fill WorkflowTask.args when we insert it into a workflow
Sounds good to me!

Ref:

@tcompa tcompa added this to the v0.6 milestone May 17, 2023
@tcompa
Copy link
Collaborator Author

tcompa commented Jun 26, 2023

We realized this issue may become tricky, but the main feature is present. The rest is part of #209. Closing.

@tcompa tcompa closed this as completed Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants