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

EDU-3624: Python Project Tutorial Geocoding #295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GSmithApps
Copy link
Contributor

@GSmithApps GSmithApps commented Aug 16, 2024

What was changed

This adds a python project-based tutorial to the docs/tutorials.

Note that the accompanying code is currently in my personal repository. It would ideally be migrated to the Temporal organization along with the other project-based-tutorial code such as https://github.com/temporalio/data-pipeline-project-python and https://github.com/temporalio/email-subscription-project-python.

Why?

I asked Tom Wheeler if there was anything I could contribute, and he suggested to make a Python tutorial.

Checklist

  1. How was this tested:
    I ran yarn start in my local. There's a screenshot below showing the tutorial.

  2. Any docs updates needed?
    The only changes are to docs 👍

  3. ⚠️ Small warning: I did not clear the snippets from my code. ⚠️ I was having trouble getting the snippets after clearing them, so I figured I'd leave them in. I'm not sure if the get snips functionality is down at the moment, or if it's because the code repository is not in the Temporal organization. In any case, I saw other tutorials with the snippets still there, so I figure it's okay.

TutorialScreenshot

Copy link

vercel bot commented Aug 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
temporal-learning ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 29, 2024 11:04am

@CLAassistant
Copy link

CLAassistant commented Aug 16, 2024

CLA assistant check
All committers have signed the CLA.

@GSmithApps GSmithApps force-pushed the project-based-tutorial-python branch from c9a394d to 14b09fb Compare August 25, 2024 03:46
@GSmithApps GSmithApps force-pushed the project-based-tutorial-python branch from 14b09fb to 4258560 Compare August 25, 2024 03:53
@GSmithApps GSmithApps force-pushed the project-based-tutorial-python branch from 036402e to 6c1d20b Compare August 25, 2024 04:04
@GSmithApps GSmithApps force-pushed the project-based-tutorial-python branch 2 times, most recently from 5397b9e to 44eecf9 Compare August 27, 2024 00:45
@GSmithApps GSmithApps force-pushed the project-based-tutorial-python branch from 44eecf9 to 10a6db1 Compare August 27, 2024 01:02
@GSmithApps GSmithApps marked this pull request as ready for review August 27, 2024 01:02
@GSmithApps GSmithApps requested a review from a team as a code owner August 27, 2024 01:02
@GSmithApps GSmithApps force-pushed the project-based-tutorial-python branch from b61afa4 to c820bd6 Compare August 29, 2024 11:04
@fairlydurable fairlydurable changed the title Python Project Tutorial Geocoding EDU-3624: Python Project Tutorial Geocoding Dec 2, 2024
@axfelix
Copy link
Contributor

axfelix commented Jan 6, 2025

Thanks for this contribution! I have only a few comments/questions:

  1. Why the different make_worker and run_worker scripts? We don't tend to use this pattern anywhere, especially not in our beginner projects.
  2. Would you mind showing some example input/output at the end of the tutorial? I like to conclude these by showing the project running end-to-end.
  3. I agree it would be best to host the code repository within the Temporal org when we publish this to the learn site; do you have permissions to create a new repo yourself, so you can migrate it out of your own Github?
  4. I don't believe there's any reason you should have to use the https://python.temporal.io/temporalio.workflow.unsafe.html#imports_passed_through context manager to import your own helper functions from your Activity definition to your Workflow code... head check from @MasonEgger ?

If we get these resolved, I'd love to merge this.

@GSmithApps
Copy link
Contributor Author

Hi Alex,

Thanks so much for your kindness!

To be honest on this one, I wrote this tutorial as I was applying for this job, and I'm not sure how useful this tutorial is. I respectfully leave it up to you and your team, and if you think we don't need it, it won't hurt my feelings 😆

But if you do like it, then yeah I certainly can look into these notes in more detail and clean up the PR 👍🏼

@axfelix
Copy link
Contributor

axfelix commented Jan 7, 2025

Hey Grant!

From my point of view, it's very close to being publishable, and we could always use more tutorials! Not a high priority of course, but if you get a chance, I think it needs less than an hour of work.

@GSmithApps
Copy link
Contributor Author

Excellent - thanks Alex! Yeah I'll put it on my queue for whenever it's handy 👍

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.

4 participants