search

dapr / python-sdk

Dapr SDK for Python
Apache License 2.0
230 stars 128 forks source link

[Workflow] More workflow SDK examples #575

Closed cgillum closed 1 year ago

cgillum commented 1 year ago

Description

I've added several Python examples that correspond to the code snippets in the following documentation page: https://docs.dapr.io/developing-applications/building-blocks/workflow/workflow-patterns/

Issue reference

Related to https://github.com/dapr/docs/issues/3496

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

cgillum commented 1 year ago

@berndverst when you're back from vacation, I was hoping you can help provide me guidance on this PR. A couple initial questions I have:

  1. Where should these live? We already have a demo_workflow directory with a separate example, but the structure seems designed for only a single app whereas I need to have several apps.
  2. How should I do automated testing for these examples? I've currently tested them all manually. I'm aware that there's an automated test framework for examples, but I'm not sure how/whether they apply to directories that contain multiple apps.

Any guidance from a Python contributor would be appreciated.

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (010a5b3) 86.40% compared to head (699bb54) 86.40%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #575 +/- ## ======================================= Coverage 86.40% 86.40% ======================================= Files 74 74 Lines 3605 3605 ======================================= Hits 3115 3115 Misses 490 490 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

berndverst commented 1 year ago

The primary purpose of the examples folder in Python is actually to be an integration test! This is done via the tool mechanical-markdown. See the Readme.md file for the annotations which assert the expected output etc. This ensures that all of our examples always work as we make changes to the Python SDK!

Ideally you just expand the existing demo-workflow example. Another folder / example would mean increasing the time it takes to run these tests, and I would like to avoid that.

Another option is to add new example folders, but we would not add these to be run as integration tests. The downside there is that we'd have no way to ensure we don't break these examples in the future - and nobody will be manually running the examples as part of the release process (we do not need manual verification today and we don't want to be adding such a step). Perhaps that is ok.

I think what I would like to see in this PR is for your "examples" to be actually used in an end to end scenario that is run via mechanical-markdown. @RyanLettieri and @DeepanshuA can help you here as they have gone through this exercise recently for workflows.

EDIT: If the goal is for the example to be essentially documentation, please move this to https://github.com/dapr/python-sdk/tree/master/daprdocs/content/en/python-sdk-docs/python-sdk-extensions instead.

berndverst commented 1 year ago

@cgillum or a completely different idea: Just create a docs page for the authoring extension here and add those code examples here: https://github.com/dapr/python-sdk/tree/master/daprdocs/content/en/python-sdk-docs/python-sdk-extensions

This is what powers the docs here: https://docs.dapr.io/developing-applications/sdks/python/python-sdk-extensions/

In that case the code should not be in the examples folder.

hhunter-ms commented 1 year ago

@cgillum friendly bump

dapr-bot commented 1 year ago

This pull request has been automatically marked as stale because it has not had activity in the last 60 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

dapr-bot commented 1 year ago

This pull request has been automatically closed because it has not had activity in the last 67 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cgillum commented 1 year ago

@berndverst sorry for letting this PR go stale. I ended up going on vacation and didn't get around to picking this back up until now. Would you mind reopening this PR so that I can complete it?

My hope was to have some automated testing of these snippets so that we can be confident that the patterns in the doc article I linked to is always known to be working. With that in mind, I think the mechanical markdown option might make the most sense. I assume that content under daprdocs/content/en/python-sdk-docs/python-sdk-extensions would not have any automatic validation?

berndverst commented 1 year ago

As discussed in chat, feel free to add examples to the examples folder. However, choose a single example that will be automatically run by CI on every PR. Today that is the demo_workflow examples. This is because our CI task for example validation already takes a very long time.

Here is where the examples run on CI are defined. https://github.com/dapr/python-sdk/blob/master/tox.ini#L39

You can either leave demo_workflow there, or change this to a different example - but please do not add a new example here. I recommend that the example you choose is representative of the most important functionality you need to regularly test via CI. And for optimization sake - please do not run workflows in CI that have a lot of sleep statements and therefore hold up CI.

We can later ensure the other examples are run on a scheduled basis, and maybe also via a slash command like /ok-to-test-workflow or /test-workflow

berndverst commented 1 year ago

By the way, if you do want an example to be run automatically via our mechanical markdown tool - you need to create a README.md -- and add the right metadata. Check out some of our other examples and read about it: https://github.com/dapr/mechanical-markdown

cgillum commented 1 year ago

I went ahead and moved the samples to live under the workflow extension directory. I'm keeping the existing examples directory untouched. However, I didn't go as far as to enable mechanical markdown on the new examples since I'm not confident that it can support the interactive nature of two of the examples (I didn't do the research to confirm this yet). We'll need to eventually come up with a plan for how to keep these examples up to date, but they won't impact the existing CI.

cgillum commented 1 year ago

@berndverst I went ahead and moved these back under the root examples directory per your suggestion.

The README.md doesn't have any mechanical markdown currently. I can add it, but there are a couple challenges:

  1. The output of these workflows is non-deterministic (some of the data is random, like the workflow IDs)
  2. Two of the samples require interaction from the user (example, pressing [ENTER] in the terminal), which makes automation even more complicated - wasn't sure if Mechanical Markdown could handle this.

Let me know if I should revisit this for this PR. If MM can handle validation via regex expressions, then we can at least automate the first two examples.

berndverst commented 1 year ago

Merging this, with the expectation that these tests will not be runnable by any automation.

We will also not be manually validating these examples for future releases (that's why we have automation to begin with). In the future you might want to move the critical features you wish to verify into a single example to be run by CI/CD as validation.

For now this is fine to merge.