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
cli: addition of parameters in workflow_start() #151
cli: addition of parameters in workflow_start() #151
Conversation
50daa3d
to
38a46e6
Compare
To test running a workflow without caching: |
"required": false, | ||
"schema": { | ||
"properties": { | ||
"input_parameters": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this schema being used? I just see a free dict here:
parsed_parameters = {'parameters':
dict(p.split('=') for p in parameters)}
If this is the desired schema, we have to think exactly where we want each parameter to end up and not mix responsibilities. As I see in RWES
PR, this parameters are not just "parameters", they are "workflow_engine_parameters" as in cwltool
could be the --timestamps
flag (what I guess run_parameters
means) but they are passed as workflow parameters.
If we want also the "workflow_parameters" (i.e. in demo root6-roofit events
) (what I guess input_parameters
are) to be overwritten by this call, then we have to update the database since they are written to the DB at creation time.
In any case they are two different things we should differentiate. To keep it simple, it the PUT /api/workflows/{workflow_id_or_name}/status
I would for now, only pass "workflow_engine_parameters" and add a new parameter to the Celery task and this one would be the one representing workflow engine parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema needs updating, regarding the differentiation I can see input parameters (number of events for example) and workflow run parameters (caching true|false), the schema was created with these 2 types, but I went with a single dictionary for now, to keep it short.
We should ticketize this splitting parameters into 2, but since now only the CACHING=true|false
is read from them we can use it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense 👍 . So for now we implement this just with workflow engine parameters and we add this additional parameter in the task (engine_parameters
or similar). Later we think about how we want to handle the workflow_parameters
(I will create issue from this discussion).
I would emphasize in those two names or similar since: one, the workflow_parameters
is as we have modeled it (or input_parameters
as it is common terminology in yadage
and CWL
), and second the workflow engine parameters (related to reanahub/reana#73 (comment) dicussion), since they are literally parameters for the concrete workflow engine implementation (i.e. cwltool
ones or yadage
ones).
* ADD arbitrary number of parameters in workflow_start(). * Updates the reana-server openapi spec with parameters in set_workflow_status, and the status passed in the query instead of the request body. Signed-off-by: Dinos Kousidis <dinos.kousidis@cern.ch>
38a46e6
to
0b9684e
Compare
ADD arbitrary number of parameters in workflow_start().
Updates the reana-server openapi spec with parameters in
set_workflow_status, and the status passed in the query instead of
the request body.
Connects #21
Closes #143
Signed-off-by: Dinos Kousidis dinos.kousidis@cern.ch