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

add localmod storage #3351

Merged
merged 7 commits into from
Oct 1, 2020
Merged

add localmod storage #3351

merged 7 commits into from
Oct 1, 2020

Conversation

jacksund
Copy link

Summary

This is the start to a LocalModule storage class. It is currently working with Prefect Server along with a LocalAgent, but full testing and features have not been added yet. Thus, this is a draft PR for discussion of code. Prior discussion of this topic is within #3304 and #3335 threads.

Changes

  • adds localmod.py (with LocalModule class) to prefect.environments.storage
  • adds LocalModule storage schema to prefect.serialization.storage
  • adds LocalModule storage as a default label and support for LocalAgent
  • adds tests for the above changes [incomplete at the moment]

Importance

Local module based storage allows for easier sharing of workflow libraries, where flows are accessible in the python path. LocalModule will serve just like Local with stored_as_script=True, where instead of a path='/path/to/my/flows/exampleflow.py', you could use module_path='from myrepo.flows import exampleflow' or 'myrepo.flows.exampleflow'. Thus the flow is accessible in any environment where that module import works, not just the environments that share the same path structure.

Checklist

NOTE -- this is a draft PR so the tasks below are still ongoing.

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

This is the start to a LocalModule storage class. It is currently working with Prefect Server along with a LocalAgent, but full testing and features have not been added yet.
@marvin-robot
Copy link
Member

Here I am, brain the size of a planet and they ask me to welcome you to Prefect.

So, welcome to the community @jacksund! 🎉 🎉

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

Hi @jacksund - first off, apologies for the extended delay on reviewing. Now that I've seen your implementation (which is great!) I realize that we don't need to create an entirely new class for this storage pattern.

In particular, I think we can expand the types of location strings we accept on the pre-existing Local storage class to allow for module imports instead of file paths. And note that we will only use this functionality when store_as_script=True. In that case we can do something like

module_path, class_str = location.split(".")[:-1], location.split(".")[-1]

and recreate your functionality here.

Let me know what you think about that, and apologies for not identifying this earlier!

@jacksund
Copy link
Author

jacksund commented Oct 1, 2020

No worries on the slow reply :). And yeah, I can definitely merge the functionality, so I'll give this a go.

Thanks for taking a look!

@jacksund
Copy link
Author

jacksund commented Oct 1, 2020

Alright so I have a working version if you take a look at my branch again. It ended up being extremely little code, so it's nice and easy to read through. There are a couple things to note though: (1) I haven't updated the doc-strings to describe the new functionality, (2) I didn't change the 'path' parameter to 'location' as you suggest because I'm scared that will cause breaks in the code elsewhere.

While everything looks to be running correctly on my end, I'm completely new to circleci, so I don't understand the build errors that I'm getting. Would you be willing to walk me through these?

@jacksund jacksund requested a review from cicdw October 1, 2020 03:58
@marvin-robot
Copy link
Member

Here I am, brain the size of a planet and they ask me to welcome you to Prefect.

So, welcome to the community @jacksund! 🎉 🎉

@codecov
Copy link

codecov bot commented Oct 1, 2020

Codecov Report

Merging #3351 into master will decrease coverage by 0.06%.
The diff coverage is 46.66%.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

Two minor requests.

Your CircleCI job is faliing because of our formatting rules - if you install and run black on the codebase you should be good; here's how to do that from the CLI:

pip install -U black
black . # note the period

Otherwise, if you could add a changelog entry to the changes/ directory with your contributor info I think we're good to go!

src/prefect/utilities/storage.py Outdated Show resolved Hide resolved
src/prefect/utilities/storage.py Outdated Show resolved Hide resolved
@jacksund
Copy link
Author

jacksund commented Oct 1, 2020

I think that covers the docs and typos. Let me know if I need to do anything else.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

LGTM thank you for the cool contribution @jacksund ! Hope it's your first of many :D

@cicdw cicdw merged commit ab53f59 into PrefectHQ:master Oct 1, 2020
@m1so m1so mentioned this pull request Oct 9, 2020
3 tasks
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