-
Notifications
You must be signed in to change notification settings - Fork 199
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
Improve documentation about configuration and workflows #2919
Conversation
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.
LGTM.
Thanks for this. One additional thing this helps with is that there was previously no mention of |
@Andrew-S-Rosen , do the new explanations about imports make sense to you? |
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.
@WardLT: This looks good to me! The point about the type-hints is especially subtle, so I'm glad that was highlighted.
There is some overlap thematically with the section above. It's not a problem, but I just wanted to bring that up as well.
docs/userguide/apps.rst
Outdated
Imports and Global Variables | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
||
Parsl apps have access to less information from the script that defined then |
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.
"that defined then" --> "that defined them"
docs/userguide/apps.rst
Outdated
|
||
Parsl apps have access to less information from the script that defined then | ||
than functions run via Python's native multiprocessing libraries. | ||
The reasons is that functions are executed on workers that |
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 reasons is" --> "The reason is"
Thanks! I'll fix my grammar and de-duplicate some. Better to keep the docs shorter |
There are actually a fair amount of duplication in this section. I'm going to clean it out while it's on my attention |
- Deduplicated descriptions of rules for functions - Separated discussions of rules for function bodies from inputs/outputs - Made more section heads, bulleted lists for quicker scanning
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.
Thanks for doing this, @WardLT! Here are some minor suggestions. Feel free to accept, reject, or do whatever with it.
docs/userguide/apps.rst
Outdated
or external Bash shell code that can be asynchronously executed. | ||
An **App** defines a computation that will be executed asynchronously by Parsl. | ||
Apps are Python functions marked with a decorator which | ||
designates that the function will run asynchronously and cause it return |
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.
"it return" --> "it to return"
docs/userguide/apps.rst
Outdated
Python apps encapsulate pure Python code, while Bash apps wrap calls to external applications and scripts, | ||
and Join apps allow composition of other apps to form sub-workflows. | ||
- ``@python_app``: Most Python functions | ||
- ``@bash_app``: A python function which returns a command line program to execute |
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.
"A python" --> "A Python"
docs/userguide/apps.rst
Outdated
and Join apps allow composition of other apps to form sub-workflows. | ||
- ``@python_app``: Most Python functions | ||
- ``@bash_app``: A python function which returns a command line program to execute | ||
- ``@join_app``: A function which returns launches other apps |
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.
"which returns launches" --> "which returns one or more Apps"
docs/userguide/apps.rst
Outdated
|
||
.. code-block:: python | ||
Python Apps run Python functions. The code inside a function marked by ``@python_app`` will run on a remote system. |
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.
"will run on a remote system" --- of course, in this example things are local. Might be better to say something like "is what will be executed either locally or on the remote system"
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 point.
docs/userguide/apps.rst
Outdated
|
||
Serializing Functions from Libraries | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
The above rules do not apply for functions defined in Python modules. |
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.
Recommended change:
The above rules assume that the user is running the example code from a standalone script or Jupyter Notebook.
Functions that are defined in an installed Python module do not need to abide by these guidelines, as they are sent to workers differently than functions defined locally within a script.
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.
That does read much better, thanks.
docs/userguide/apps.rst
Outdated
|
||
Parsl can create Apps directly from functions defined in Python modules. | ||
Supply the function as an argument to ``python_app`` rather than creating a new function which is decorated. | ||
Supply a module function as an argument to ``python_app`` rather than creating a new function which is decorated. |
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 mean, there is technically nothing stopping someone from defining a new function. It's just silly.
Suggestion:
To transform an imported function into an App, you do not need to create a new function to decorate. Rather, you can do the following:
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 call. I changed the wording to reflect that it isn't necessary to define an own function
@@ -125,10 +118,67 @@ to include attributes from the original function (e.g., its name). | |||
my_max_app = python_app(my_max) |
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.
This is equivalent to doing:
.. code-block:: python
@python_app
my_max_app(*args, **kwargs):
return my_max(*args, **kwargs)
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.
Very true. Nice way of tying it to something people would already think of
docs/userguide/apps.rst
Outdated
Special Keyword Arguments | ||
+++++++++++++++++++++++++ | ||
|
||
Parsl apps can use the following special reserved keyword arguments: |
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.
This can use an example to highlight that this is referring to the decorator kwargs.
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.
Absolutely, and I also a added a section which differentiates "function" and "decorator" kwargs.
- Grammar improvements - More examples - Differentiate kwargs for function and app Thanks, @Andrew-S-Rosen
from parsl.config import Config | ||
import parsl | ||
|
||
def make_config() -> Config: |
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.
in tests and a few other places this is called fresh_config()
Description
Clarifies two problems users found in the documentation
ParslPoolExecutor.map
parsl.clear()
results inKeyError: 'block_id'
exception #2871 : Configs may not be re-usedinputs
andoutputs
to take immutable types #2925 : Explain mutable types are undesirableType of change
Choose which options apply, and delete the ones which do not apply.