-
Notifications
You must be signed in to change notification settings - Fork 189
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
Docs: Add "How to extend workflows" section #4562
Docs: Add "How to extend workflows" section #4562
Conversation
Yes, this is true. Would moving the part that corresponds to the "Topics" section involve a lot of work? Perhaps it would be better to just put this content in the new locations even if it is not perfectly adapted yet, just so that we can delete the old files.
I'm not sure I understand this question. Where exactly are you referring? |
I think both sections can serve a purpose:
I've just made an issue about this question: #4571. Have a look and let me know if it's clear. 😅 @sphuber mentioned that he won't be able to review this PR this week, as he's quite busy. Do you maybe have time to look at the "How to extend workflows" section? I'll update the "Topics - Workflows - Usage" section asap. |
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.
Great work @mbercx ! I have a couple of specific comments below, but maybe a more general one here:
I feel like including the "how to extend workchains" inside of a "how to run workflows" section kind of hides it a little bit. I now don't remember if we had any specific reason to put it here or just didn't think much about it. I would maybe just take out the last two sub-sections into a separate "How to write and extend workflows" top level section.
We can leave the "How to run multi-step workflows" for now also, but I think eventually we should review it together with the "How to run external codes" and maybe merge them (or at least re-distribute the information), since they have a lot of similar stuff.
Btw, I would also re-order the how-to list as:
...
* How to run external codes
* How to run multi-step workflows [MOVED HERE]
* How to setup SSH connections
* How to write a plugin for an external code
* How to write and extend workflows [NEW]
* How to work with data
* How to explore the provenance graph
...
Thanks for the review, @ramirezfranciscof! I've left some comments unresolved so you can respond before I make another round of changes. I think the failing tests are related to the rather outdated branch. Will rebase once we're settled on the content. |
@mbercx feedback processed |
@ramirezfranciscof I've dealt with the remaining comments. Re the changes in Re the reorganisation of the How-to section: I agree with your suggestion. However, since this also affects other sections, maybe we should also get the input of others and do this in a separate PR? Finally, there is still issue #1927. This actually relates to the "Topics - Workflows - Usage" section, so I'd have to review that section again. Either we still keep this PR open to deal with that, or we merge and I'll open another PR where I give the section another pass and fix #1927 in the process. |
Re the comment in this line: I understood from @sphuber comment that in the end this was not the case and although not submitable, the workfunctions could be set as plugin before v1.5. Did you understand otherwise? I now see he didn't explicitly say so but I assumed the explanation pointed to this...
What other sections does it affect? It splits this section and moves one half to a different position. I mean, we can check with the others if you feel like is a relevant change to the design of the docs (personally I think it is small enough to just do it), but I wouldn't say it affects other sections.
Yes, I agree that this can be a separate PR, this is substantial enough. |
Righto, I may have been a little quick in parsing @sphuber's comment. Not sure at which point entry points were introduced for calculation functions (or if this even was later than for calculation jobs?) . If this has been a while ago, maybe it makes sense just to remove the note?
👍 Will go ahead with the move then.
👍 |
Yeah, I agree that we just maybe remove that one if we are not sure. I'm removing the draft status, ping me again when you are ready for a (hopefully) final review. |
0aa1ea0
to
3caa87a
Compare
@ramirezfranciscof the sections have been reorganised and I've removed the first note regarding the change in v1.5.0 which we were not sure of. Let me know if you still have comments! |
fd07fa2
to
512230b
Compare
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.
I just had a couple of comments but they are not even that critical, it should be good to go once you update with the parent branch @mbercx .
Add a section on how to extend workflows to the "How to run multi-step workflows" section. This section continues with the `MultiplyAddWorkChain` example and covers: * How to submit the `MultiplyAddWorkChain` within a parent work chain. * How to expose the inputs using the `expose_inputs` method and a proper namespace. * How to use the exposed inputs with the `exposed_inputs` method. * How to expose outputs and pass them to the outputs of the parent work chain.
Co-authored-by: ramirezfranciscof <[email protected]>
Co-authored-by: ramirezfranciscof <[email protected]>
I missed this one in when dealing with the merge conflict. Should be the only difference."
fc392e1
to
d383c1e
Compare
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.
Good to go!
Hmm, seems there are tests that don't really need to be run for the docs, but aren't skipped. I guess you can merge with admin rights, but strange that this is an issue again? |
@mbercx This might have been because of the recent problem with the dependencies (see #4850)? I updated the recent fix, check if you can merge after those test pass and if not let me know what you want the commit message to be and I'll merge it (also, if this is the case perhaps we should open an issue or ask if there is a way to set this up better). |
Fixes #3993
Split the section on "How to run multi-step workflows" into one that focuses on running the workflows and one on "How to write and extend workflows". Add a subsection on how to extend workflows to the second.
This subsection continues with the
MultiplyAddWorkChain
example and covers:MultiplyAddWorkChain
within a parent work chain.expose_inputs
method and a proper namespace.exposed_inputs
method.TODO
future
variable which stores the output ofself.submit
to something that shows clearly that this is just the process node of the process being submitted.MultiplyAddWorkChain
returns an exit code because the result is negative. What would be the recommended way to handle this?Notes/questions
process_class
to theexposed_outputs
method? The first argument of this method already is the process node, so can't we just extract theprocess_class
usingnode.process_class
?