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

Docs: Add "How to extend workflows" section #4562

Merged

Conversation

mbercx
Copy link
Member

@mbercx mbercx commented Nov 13, 2020

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:

  • 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.

TODO

  • Consider replacing the future variable which stores the output of self.submit to something that shows clearly that this is just the process node of the process being submitted.
  • Deal with the issues in Questions/suggestions on documentation page about workflows #1927. This might still require some discussion on what to do with the "Topics - Workflows - Usage" section.
  • Deal with the case where the MultiplyAddWorkChain returns an exit code because the result is negative. What would be the recommended way to handle this?

Notes/questions

  • There's quite a bit of overlap between the "How to run multi-step workflows" and "Topics - Workflows - Usage" sections. The way I see it, this is not necessarily a problem. We can leave the how-to section as an introduction, and explain the various elements to writing a work chain in more detail in the Topics section. However, we should at some point give the latter another pass and consider updating the examples, as well as check if it is complete, i.e. deals with all aspects of writing workflows.
  • Why do we need to pass the process_class to the exposed_outputs method? The first argument of this method already is the process node, so can't we just extract the process_class using node.process_class?

@ramirezfranciscof
Copy link
Member

There's quite a bit of overlap between the "How to run multi-step workflows" and "Topics - Workflows - Usage" sections.

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.

Why do we need to pass the process_class to the exposed_outputs method? The first argument of this method already is the process node, so can't we just extract the process_class using node.process_class?

I'm not sure I understand this question. Where exactly are you referring?

@mbercx
Copy link
Member Author

mbercx commented Nov 17, 2020

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 think both sections can serve a purpose:

  • The "How to" sections are meant to be concise rather than complete, so the current content (including that of this PR) is probably sufficient.
  • The "Topics" section should aim to be complete in explaining the various concepts and design decisions, as content for more experienced users. It already does this to some extent, but we should probably update/tests the examples here in a next round of documentation days, as well as check if there are any topics that should still be explained here. That's quite a bit of work though, so for now I'll just address the issues in Questions/suggestions on documentation page about workflows #1927 in this PR.

I'm not sure I understand this question. Where exactly are you referring?

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.

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a 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
...

docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/workflows.rst Outdated Show resolved Hide resolved
@mbercx
Copy link
Member Author

mbercx commented Jan 26, 2021

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.

@ramirezfranciscof
Copy link
Member

@mbercx feedback processed

@mbercx
Copy link
Member Author

mbercx commented Jan 27, 2021

@ramirezfranciscof I've dealt with the remaining comments.

Re the changes in v1.5.0, I've left the two comments instead of merging them somewhere into one. They were quite separated in the text, so it would be strange to only mention the note on the entry point when discussing the submission to the daemon, for example.

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.

@ramirezfranciscof
Copy link
Member

ramirezfranciscof commented Jan 27, 2021

@ramirezfranciscof I've dealt with the remaining comments.

Re the changes in v1.5.0, I've left the two comments instead of merging them somewhere into one. They were quite separated in the text, so it would be strange to only mention the note on the entry point when discussing the submission to the daemon, for example.

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...

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?

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.

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.

Yes, I agree that this can be a separate PR, this is substantial enough.

@mbercx
Copy link
Member Author

mbercx commented Jan 27, 2021

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...

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?

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.

👍 Will go ahead with the move then.

Yes, I agree that this can be a separate PR, this is substantial enough.

👍

@ramirezfranciscof
Copy link
Member

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?

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.

@mbercx
Copy link
Member Author

mbercx commented Feb 3, 2021

@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!

@mbercx mbercx force-pushed the fix/3993/howto_extend_workflows branch from fd07fa2 to 512230b Compare February 17, 2021 20:29
Copy link
Member

@ramirezfranciscof ramirezfranciscof left a 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 .

docs/source/howto/write_workflows.rst Outdated Show resolved Hide resolved
docs/source/howto/write_workflows.rst Show resolved Hide resolved
docs/source/howto/write_workflows.rst Outdated Show resolved Hide resolved
mbercx and others added 6 commits April 8, 2021 11:50
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 <ramirezfranciscof@users.noreply.github.com>
mbercx and others added 3 commits April 8, 2021 11:57
Co-authored-by: ramirezfranciscof <ramirezfranciscof@users.noreply.github.com>
I missed this one in when dealing with the merge conflict.
Should be the only difference."
@mbercx mbercx force-pushed the fix/3993/howto_extend_workflows branch from fc392e1 to d383c1e Compare April 8, 2021 10:02
@mbercx mbercx requested a review from ramirezfranciscof April 8, 2021 10:02
Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Good to go!

@mbercx
Copy link
Member Author

mbercx commented Apr 8, 2021

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?

@ramirezfranciscof
Copy link
Member

@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).

@ramirezfranciscof ramirezfranciscof merged commit 5ab86cc into aiidateam:develop Apr 9, 2021
@mbercx mbercx deleted the fix/3993/howto_extend_workflows branch April 9, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: How to extend workflows
3 participants