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

Default to Docker() storage? #1044

Closed
cicdw opened this issue May 17, 2019 · 5 comments
Closed

Default to Docker() storage? #1044

cicdw opened this issue May 17, 2019 · 5 comments
Assignees
Labels
enhancement An improvement of an existing feature

Comments

@cicdw
Copy link
Member

cicdw commented May 17, 2019

Question: now that initializing Docker storage sets sensible defaults for base_image and prefect_version, and additionally installs kubernetes extras, should we default to an unmodified Docker() class for every Flow? I.e.,

f = Flow()
f.storage # populated with Docker() storage

I don't believe this has any consequences for users not deploying their Flows to Cloud (because flow.serialize defaults to using build=False), but does allow simple Flows with no extraneous dependencies to be deployed with ease.

This would require us to introduce one additional configuration setting for the registry_url, as we don't want to set any public defaults here, but could read a default value from a user's config or environment variable.

cc: @jlowin @joshmeek

@cicdw cicdw added the enhancement An improvement of an existing feature label May 17, 2019
@jlowin
Copy link
Member

jlowin commented May 17, 2019

Makes sense to me.

I'm concerned that we're starting to have a creeping pattern of "we want this thing to be instantiated automatically, but we need to provide init arguments" and our config is going to become crowded (and stale). Perhaps there's a clean way to mark init arguments as being read from config values (or, more likely, env vars), similar to how we use default_from_attrs to mark run arguments as being read from attrs.

Something like:

class MyClass:
	@defaults_from_config(a="defaults.myclass.a", b="another_thing.myclass.b")
    def __init__(self, a=1, b=2):
        pass

This way, a and b have Pythonic defaults, but can use prefect.config.defaults.myclass.a and prefect.config.another_thing.myclass.b if those keys are defined.

Maybe too crazy / overengineered?

@cicdw
Copy link
Member Author

cicdw commented May 18, 2019

Hmmmmm I'm definitely intrigued by this. So the idea here is that we don't have to populate these default values in our config.toml file because they are set in code, however they can be overwritten via user config if desired?

@joshmeek
Copy link

Yeah I'm on board with this

@cicdw
Copy link
Member Author

cicdw commented Aug 8, 2019

OK so I decided I'm uncomfortable with a default registry_url for security reasons. However, I have a new idea:

  • have the default storage class be configurable from config.toml
  • let flow.deploy accept arbitrary **kwargs that are then passed into the initialization method of the default storage which is only attached to the flow at deploy time

This would allow for the following lightning fast deploy:

from prefect import Flow

Flow("speedy").deploy("My Project", registry_url="cicdw", image_name="speedy-flow")

cc: @joshmeek @jlowin

@cicdw cicdw self-assigned this Aug 8, 2019
@cicdw
Copy link
Member Author

cicdw commented Aug 12, 2019

This was closed with #1337

@cicdw cicdw closed this as completed Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants