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

Parse flow parameter docstrings and add it to parameter description #8004

Merged
merged 14 commits into from Jan 4, 2023

Conversation

j-tr
Copy link
Contributor

@j-tr j-tr commented Dec 29, 2022

prepares fix for #3740

Adds parsing for flow docstrings in Google docstring style and saves docstring in the description field in properties of ParameterSchema.
This would make it possible to fetch the parameter docstring from the deployment endpoint and enable the display of a help text next to the parameter input field in the flow submission form in the UI as requested in #3740. Requires adaptation of the UI to display its full potential.

Example

When deploying a flow that contains a docstring in Google docstring style, the docstring for each parameter is parsed and added to the ParameterSchema, which is stored in the DB and made available via the API.

flow_with_docstring.py:

from prefect import flow


@flow()
def flow_with_docstring(a, b, c) -> None:
    """This is the first line of a docstring.

    This is some additional docstring text
    with multiple lines.

    Args:
        a (str): parameter a with type annotation
        b: parameter b without type annotation
        c: parameter c
            with indented second line
    """
    pass

$ prefect deployment build flow_with_docstring.py:flow_with_docstring --name parameter_docstring --apply
Found flow 'flow-with-docstring'
Deployment YAML created at 'flow_with_docstring-deployment.yaml'.
Deployment storage None does not have upload capabilities; no files uploaded.  Pass --skip-upload to suppress this warning.
Deployment 'flow-with-docstring/parameter_docstring' successfully created with id 'a5891141-7cec-43ef-83bc-686e7e2d2070'.
$ curl -s http://127.0.0.1:4200/api/deployments/a5891141-7cec-43ef-83bc-686e7e2d2070 | jq '.parameter_openapi_schema | .properties[] | {title, description}'
{
  "title": "a",
  "description": "parameter a with type annotation"
}
{
  "title": "b",
  "description": "parameter b without type annotation"
}
{
  "title": "c",
  "description": "parameter c with indented second line"
}

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a label categorizing the change e.g. fix, feature, enhancement

@j-tr j-tr requested a review from zanieb as a code owner December 29, 2022 12:58
@netlify
Copy link

netlify bot commented Dec 29, 2022

Deploy Preview for prefect-orion ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit c866289
🔍 Latest deploy log https://app.netlify.com/sites/prefect-orion/deploys/63b462e8a6cf8500094d2067
😎 Deploy Preview https://deploy-preview-8004--prefect-orion.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@zanieb
Copy link
Contributor

zanieb commented Dec 29, 2022

HI! This looks pretty cool and I believe it would be a welcome addition. I think that you can use griffe to parse docstrings instead of implementing it yourself, I think we do this elsewhere for blocks? @ahuang11 / @desertaxle are the experts on this.

@desertaxle
Copy link
Member

We do indeed use griffe for parsing the docstrings of Block subclasses. You can see an example of that here where we use griffe to get the description section of the docstring.

@j-tr
Copy link
Contributor Author

j-tr commented Dec 30, 2022

happy to use griffe instead. I also removed some now superfluous tests which were only testing the parser itself.

Copy link
Contributor

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Nice! Thank you!

One minor tweak: Can you change the tests in test_callables.py to be for functions that aren't flows? Our utilities aren't supposed to have knowledge of Prefect concepts (as a separation of concerns). It'd be great to have a test in test_flows.py that covers a basic case of this for a flow.

Copy link
Contributor

@ahuang11 ahuang11 left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I have a few suggestions, but looks good to me!

src/prefect/utilities/callables.py Outdated Show resolved Hide resolved
src/prefect/utilities/callables.py Outdated Show resolved Hide resolved
src/prefect/utilities/callables.py Outdated Show resolved Hide resolved
src/prefect/utilities/callables.py Outdated Show resolved Hide resolved
@j-tr
Copy link
Contributor Author

j-tr commented Dec 30, 2022

@madkinsz thanks for pointing out the issue with non-flow functions. I retrieved the docstring from the flow description which is not only problematic for non-flow functions but also if the default description is overwritten. Now retrieving it directly via inspect.
I wasn't entirely sure about the correct test class in test_flows.py, so feel free to suggest a more suitable location.

@j-tr
Copy link
Contributor Author

j-tr commented Jan 2, 2023

I'm afraid that I don't fully understand how the changes from this PR cause some unrelated tests to fail. Please let me know how I can contribute so that this can be merged.

@ahuang11
Copy link
Contributor

ahuang11 commented Jan 3, 2023

Yeah the changes from this PR are unrelated; I think it's because there was a new version of httpx that was released that broke the tests. Testing locally, pip install httpx==0.23.1 passes but pip install httpx-0.23.2 can reproduce the failures.

Can't find a mention about it though:
https://github.com/encode/httpx/releases

Oh I found it here: encode/httpx#2523

@zanieb zanieb merged commit e711ddd into PrefectHQ:main Jan 4, 2023
github-actions bot pushed a commit that referenced this pull request Jan 4, 2023
…8004)

Co-authored-by: Justin Trautmann <justin@live-eo.com>
Co-authored-by: Michael Adkins <michael@prefect.io>
github-actions bot pushed a commit that referenced this pull request Jan 4, 2023
…8004)

Co-authored-by: Justin Trautmann <justin@live-eo.com>
Co-authored-by: Michael Adkins <michael@prefect.io>
masonmenges pushed a commit that referenced this pull request Jan 10, 2023
…8004)

Co-authored-by: Justin Trautmann <justin@live-eo.com>
Co-authored-by: Michael Adkins <michael@prefect.io>
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.

None yet

4 participants