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

Update documentation to use new operation.with_directives decorator #128

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

b-butler
Copy link
Member

Description

This changes the documentation to use the FlowProject.operation.with_directives decorator over flow.directives. This is to teach a more coherent syntax with FlowGroupEntry.with_directives.

Motivation and Context

Is the corresponding documentation PR for glotzerlab/signac-flow#502.

Checklist:

@b-butler b-butler requested review from a team as code owners April 21, 2021 00:54
@b-butler b-butler requested review from atravitz and Tobias-Dwyer and removed request for a team April 21, 2021 00:54
@b-butler
Copy link
Member Author

@atravitz and @Tobias-Dwyer will you guys have time to look at this sometime next week? We just merged glotzerlab/signac-flow#502 and this goes with that.

Copy link
Collaborator

@kidrahahjo kidrahahjo left a comment

Choose a reason for hiding this comment

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

The changes look good, I checked docs.signac.io and saw that wall time and memory directives are missing. I'll make a PR that adds them.

docs/source/cluster_submission.rst Outdated Show resolved Hide resolved
docs/source/flow-group.rst Outdated Show resolved Hide resolved
docs/source/recipes.rst Outdated Show resolved Hide resolved
docs/source/recipes.rst Outdated Show resolved Hide resolved
@b-butler b-butler requested a review from bdice June 8, 2021 14:50
Comment on lines 261 to 266
.. code-block:: python

@FlowProject.operation
@FlowProject.operation.with_directives({"np": 2})
@flow.cmd
@flow.directives(np=2)
def hello_mpi(job):
return "mpiexec -n 2 mpi_program {job.ws}/input_file.txt"
Copy link
Member

Choose a reason for hiding this comment

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

Is this example still useful, or should we recommend the user to use the nranks directive and adapt the command to remove mpiexec?

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 whole section on MPI is out of date and actually would likely not work given the submit -> run -> exec model. However, I don't know if it is in scope to modify this in this PR.

docs/source/recipes.rst Show resolved Hide resolved
@bdice
Copy link
Member

bdice commented Jun 8, 2021

The changes look good, I checked docs.signac.io and saw that wall time and memory directives are missing. I'll make a PR that adds them.

@kidrahahjo Did you want to make this PR? Thanks!

@kidrahahjo
Copy link
Collaborator

@bdice #134 is ready to merge once @vyasr or @klywang approves it.

@b-butler
Copy link
Member Author

b-butler commented Jun 9, 2021

@bdice I made an issue #144 to document the needed changes to the MPI documentation in recipes. I think these changes should go in a separate PR. If you agree can you provide another review, if not then let me know, so I can make the changes here.

@b-butler b-butler requested a review from bdice June 9, 2021 15:07
@bdice bdice merged commit bf6e236 into master Jun 9, 2021
@bdice bdice deleted the update-directives-setting branch June 9, 2021 16:24
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.

3 participants