-
Notifications
You must be signed in to change notification settings - Fork 276
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
Pass in project, domain and version in register rather than serialize #288
Changes from all commits
1bfda6a
bd9cb3a
35258b3
52cc5fe
88cb26c
b96bd6b
be5c9c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,20 @@ | ||
import click as _click | ||
|
||
CTX_PROJECT = "project" | ||
CTX_DOMAIN = "domain" | ||
CTX_VERSION = "version" | ||
CTX_TEST = "test" | ||
CTX_PACKAGES = "pkgs" | ||
CTX_NOTIFICATIONS = "notifications" | ||
|
||
|
||
project_option = _click.option( | ||
"-p", | ||
"--project", | ||
required=True, | ||
type=str, | ||
help="Flyte project to use. You can have more than one project per repo", | ||
) | ||
domain_option = _click.option( | ||
"-d", "--domain", required=True, type=str, help="This is usually development, staging, or production", | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
import click | ||
|
||
from flytekit.clis.sdk_in_container.constants import CTX_DOMAIN, CTX_PACKAGES, CTX_PROJECT, CTX_VERSION | ||
from flytekit.clis.sdk_in_container.constants import CTX_PACKAGES, CTX_VERSION | ||
from flytekit.clis.sdk_in_container.launch_plan import launch_plans | ||
from flytekit.clis.sdk_in_container.register import register | ||
from flytekit.clis.sdk_in_container.serialize import serialize | ||
|
@@ -20,16 +20,6 @@ | |
|
||
|
||
@click.group("pyflyte", invoke_without_command=True) | ||
@click.option( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this means that if you have I know I said it was okay to take out project from serialize but I guess I didn't realize what that entailed. if we're going to break compatibility, should we get rid of pyflyte register entirely? Maybe let's discuss with ketan? I'm okay taking it out completely, leaving pyflyte serialize to be the one and only command you need. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kumare3 any thoughts on getting rid of pyflyte register altogether? @wild-endeavor there's also the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am all for simplifying this story, but what about existing users and their make files - let's say at lyft There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought backwards compatibility wasn't an explicit concern? If we want to go that route then we make PROJECT, DOMAIN, VERSION optional params in the pyflyte command group and every subcommand has to check that they're secretly filled in. I see it as simpler to use click features to require the args at the most specific level where they are used |
||
"-p", | ||
"--project", | ||
required=True, | ||
type=str, | ||
help="Flyte project to use. You can have more than one project per repo", | ||
) | ||
@click.option( | ||
"-d", "--domain", required=True, type=str, help="This is usually development, staging, or production", | ||
) | ||
@click.option( | ||
"-c", "--config", required=False, type=str, help="Path to config file for use within container", | ||
) | ||
|
@@ -48,7 +38,7 @@ | |
"-i", "--insecure", required=False, type=bool, help="Do not use SSL to connect to Admin", | ||
) | ||
@click.pass_context | ||
def main(ctx, project, domain, config=None, pkgs=None, version=None, insecure=None): | ||
def main(ctx, config=None, pkgs=None, version=None, insecure=None): | ||
""" | ||
Entrypoint for all the user commands. | ||
""" | ||
|
@@ -60,8 +50,6 @@ def main(ctx, project, domain, config=None, pkgs=None, version=None, insecure=No | |
_logging.getLogger().setLevel(log_level) | ||
|
||
ctx.obj = dict() | ||
ctx.obj[CTX_PROJECT] = project | ||
ctx.obj[CTX_DOMAIN] = domain | ||
version = version or _look_up_version_from_image_tag(_IMAGE.get()) | ||
ctx.obj[CTX_VERSION] = version | ||
|
||
|
@@ -78,10 +66,6 @@ def main(ctx, project, domain, config=None, pkgs=None, version=None, insecure=No | |
pkgs = _WORKFLOW_PACKAGES.get() | ||
ctx.obj[CTX_PACKAGES] = pkgs | ||
|
||
_os.environ[_internal_config.PROJECT.env_var] = project | ||
_os.environ[_internal_config.DOMAIN.env_var] = domain | ||
_os.environ[_internal_config.VERSION.env_var] = version | ||
|
||
|
||
def update_configuration_file(config_file_path): | ||
""" | ||
|
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.
do we really need these?
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.
don't we still need the topological sort to guarantee that we register tasks before workflows that reference them, and workflows before launch plans that reference them?