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

Task updating: Handle new parameters with default values #2349

Closed
jluethi opened this issue Mar 7, 2025 · 4 comments
Closed

Task updating: Handle new parameters with default values #2349

jluethi opened this issue Mar 7, 2025 · 4 comments
Assignees
Labels

Comments

@jluethi
Copy link
Collaborator

jluethi commented Mar 7, 2025

@rhornb & @adrtsc discovered an issue with the way we handle new parameters getting added to a task & the task update logic.

Currently, if a task update add a new variable darkfield : bool = True that wasn't set in the old version, the update mode in Fractal web automatically updates the task without error messages. When one download the workflow, that variable is unset in the workflow (somewhat expected based on old task parameter logic).
Fractal web then displays an unset boolean as False. This is fairly misleading, because that parameter defaults to True, thus when the task is run, darkfield=True will be used, even though darkfield=False is shown in the web interface.

This example can be reproduced by upgrading form the APX Apply BaSiCPy Illumination Models task version 0.4.0 to 0.4.4.

Task pre upgrade (version 0.4.0):

Image

Task after upgrade from 0.4.0 to 0.44 (without any warnings):
Image

Task when added directly as version 0.4.4:
Image

A similar issue can also be observed when other parameters with default values (e.g. the "correct_by" variable in screenshots above) get added.

Content of the workflow download

{
  "name": "Update_testing",
  "task_list": [
    {
      "meta_non_parallel": null,
      "meta_parallel": {
        "cpus_per_task": 1,
        "mem": 3750
      },
      "args_non_parallel": null,
      "args_parallel": {
        "illumination_profiles_folder": "test",
        "input_ROI_table": "FOV_ROI_table",
        "overwrite_input": true,
        "suffix": "_illum_corr"
      },
      "type_filters": {},
      "task": {
        "pkg_name": "apx-fractal-task-collection",
        "version": "0.4.0",
        "name": "Apply BaSiCPy Illumination Models"
      }
    },
    {
      "meta_non_parallel": null,
      "meta_parallel": {
        "cpus_per_task": 1,
        "mem": 3750
      },
      "args_non_parallel": null,
      "args_parallel": {
        "illumination_profiles_folder": "Test",
        "input_ROI_table": "FOV_ROI_table",
        "overwrite_input": true,
        "suffix": "_illum_corr"
      },
      "type_filters": {},
      "task": {
        "pkg_name": "apx-fractal-task-collection",
        "version": "0.4.4",
        "name": "Apply BaSiCPy Illumination Models"
      }
    },
    {
      "meta_non_parallel": null,
      "meta_parallel": {
        "cpus_per_task": 1,
        "mem": 3750
      },
      "args_non_parallel": null,
      "args_parallel": {
        "illumination_profiles_folder": "test2",
        "correct_by": "channel label",
        "darkfield": true,
        "baseline": true,
        "input_ROI_table": "FOV_ROI_table",
        "overwrite_input": true,
        "suffix": "_illum_corr"
      },
      "type_filters": {},
      "task": {
        "pkg_name": "apx-fractal-task-collection",
        "version": "0.4.4",
        "name": "Apply BaSiCPy Illumination Models"
      }
    }
  ]
}

(when reproducing, one needs to save the task with required parameters before the workflow download has any content in the args)

@jluethi
Copy link
Collaborator Author

jluethi commented Mar 7, 2025

I think we should handle this better during upgrading. Our current logic still relies heavily on the idea that only some parameters would get set, but we've switched to always setting all parameters when more of the upgrade handling moved to the backend, right?
If, during upgrading, we'd add the newly added parameters with their defaults, that would work correctly, both for booleans and strings.

@jluethi
Copy link
Collaborator Author

jluethi commented Mar 7, 2025

Another way to reword this: Parameters with a default value shouldn't end up in a None state (=> greyed out) in regular Fractal web usage. And current update procedures sometimes get us to that state

@tcompa tcompa added the API label Mar 11, 2025
@tcompa tcompa assigned ychiucco and tcompa and unassigned ychiucco Mar 11, 2025
@tcompa tcompa linked a pull request Mar 12, 2025 that will close this issue
3 tasks
@tcompa
Copy link
Collaborator

tcompa commented Mar 12, 2025

I'm making a small change as part of #2355, but it's not meant to fix this issue.

I think it has to do with the webclient logic in https://github.com/fractal-analytics-platform/fractal-web/blob/2939112ef3890463f63dad6d7e56da2153b6ad5c/src/lib/components/v2/workflow/VersionUpdate.svelte#L155-L179.
TBD

@tcompa
Copy link
Collaborator

tcompa commented Mar 13, 2025

Now tracked in fractal-analytics-platform/fractal-web#728

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

3 participants