-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Storage Wars #956
Storage Wars #956
Conversation
self.image_tag, # type: ignore | ||
) | ||
|
||
def __contains__(self, obj: Any) -> bool: |
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 was also a really good idea!
env_vars=env_vars, | ||
) | ||
) | ||
|
||
dockerfile.write(file_contents) | ||
|
||
def create_dockerfile_object_from_dockerfile( |
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.
What's the intention behind removing the entire full dockerfile option? Too many extra required steps i.e. healthcheck, copying flows, etc...?
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.
So the intent was to simplify the interface for now, and the only difference between create_dockerfile_object_from_dockerfile
and the current method is that the current method's docker file is:
FROM {base_image}
# DO pip stuff
so I figured when we need this feature we can easily generalize our current method (and avoid having two different methods).
This is awesome! |
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 think this looks great!
One suggestion is perhaps to change the signature of get_flow_location(flow: Flow)
to get_flow_location(flow_name: str)
. The reason is that in almost all circumstances, get_flow_location
will be called from a different process than the one that built the storage, and so the Flow
object will be necessarily different (in a "Python ID" sense) than the one that was stored. Therefore a string attribute like its name or some other key will ultimately be the only way to identify it to the Storage. (Memory
seems to be an exception to this rule, but would be compatible with using the name as a key instead of the flow itself). Otherwise this will require instantiating a Flow object just to use Storage which is supposed to contain the Flow... and that feels circular.
Co-Authored-By: cicdw <white.cdw@gmail.com>
@jlowin happy to update the signature of that method. Just for the record, the only reason I left it like that was in case new types of storage relied on other permanent attributes of Flows other than name, but I do think it's better to keep it simple for now. |
This is such a clean implementation and advances so many of our goals that we thought would take longer, so let's start with the simpler version to get off the ground |
@jlowin and @joshmeek : I've addressed all of your feedback and updated environment = flow.environment
storage = flow.storage
environment.setup(storage)
environment.execute(storage, storage.flows[flow.name]) |
Actually, writing that makes me wonder whether |
Thanks for contributing to Prefect!
Please describe your work and make sure your PR:
CHANGELOG.md
(if appropriate)docs/outline.toml
for API reference docs (if appropriate)Note that your PR will not be reviewed unless all three boxes are checked.
What does this PR change?
This PR builds off @joshmeek 's work and beefs up our
Storage
classes as follows:Memory
storage option for local testingStorage
interface for storing multiple flows, and accessing flows contained in Storagestorage
submodule to our exposed API documentationI also removed a few things from the initial implementation for the sake of simplicity / exposing a cleaner interface until we know how we want these things to be used.
Assuming this PR is approved and merged, it would be very easy from here to implement a
LocalEnvironment
and aDaskClusterEnvironment
using this interface. I avoided taking it that far because it's already a large PR.For the full details of what this PR is trying to achieve, please read the PIN-7 that is included.