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

Handle input params correctly for rerun_invocation() #395

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

simonbray
Copy link
Member

No description provided.

@simonbray simonbray changed the title [WIP] Add remap option to rerun_invocation() method Add remap option to rerun_invocation() method May 19, 2021
invocation_details = self.show_invocation(invocation_id)
workflow_id = invocation_details['workflow_id']
inputs = invocation_details['inputs']
wf_params = invocation_details['input_step_parameters']
step_ids_to_indices = {step['workflow_step_id']: step_index for step_index, step in enumerate(invocation_details['steps'])}
inputs.update({step_ids_to_indices[param['workflow_step_id']]: param['parameter_value'] for _, param in invocation_details['input_step_parameters'].items()})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsoranzo I found that the params option was not working correctly so I removed it (as params can also be submitted via inputs) - does this look like an okay alternative?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh, what's not working for you? I notice two issues:

  • invocation_details['input_step_parameters'] seems to be never populated by Galaxy, so params_update should actually be just params as in invoke_workflow()
  • payload = {'inputs': inputs, 'params': wf_params} should have been payload = {'inputs': inputs, 'parameters': wf_params}

Does that help?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invocation_details['input_step_parameters'] seems to be never populated by Galaxy, so params_update should actually be just params as in invoke_workflow()

It does, but only if the input parameter is labelled.

payload = {'inputs': inputs, 'params': wf_params} should have been payload = {'inputs': inputs, 'parameters': wf_params}

Whoops, maybe that was it - I'll try that, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsoranzo it seems that params doesn't work with runtime parameters, can you confirm if that's the case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @mvdbeek

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least I couldn't get it to work with a runtime parameter, and the documentation:

        The ``params`` dict should be specified as follows::

          {STEP_ID: PARAM_DICT, ...}

        where PARAM_DICT is::

          {PARAM_NAME: VALUE, ...}

doesn't really make sense for a runtime parameter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make it clear that runtime parameters belong in inputs, not parameters: https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/workflow/run_request.py#L282-L285

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsoranzo would it be okay if we merge this? I would like to finish https://github.com/galaxyproject/planemo/pull/1140/files and I feel this should be completed first.

I am pretty confident that runtime parameters can only be submitted via inputs, see the two comments above. Also that's how Planemo is doing it: https://github.com/galaxyproject/planemo/blob/master/planemo/galaxy/activity.py#L165

As a result these two lines are needed to 1) get the runtime parameters from the original invocation's input_step_parameters 2) use the workflow_step_ids to match them with the numeric step ids. Then the numeric step ids can be used to update inputs and invoke the new workflow.

I can also split the last two commits into a separate PR if you prefer so we can merge at least the first one (the remapping bit).

@simonbray simonbray requested a review from nsoranzo May 19, 2021 08:58
@simonbray simonbray changed the title Add remap option to rerun_invocation() method Handle input params correctly for rerun_invocation() Oct 25, 2021
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

Successfully merging this pull request may close these issues.

2 participants