Pass in project, domain and version in register rather than serialize #288
Conversation
katrogan
requested review from
kumare3,
matthewphsmith and
wild-endeavor
as code owners
flytekit/clis/flyte_cli/main.py
Outdated
| id, entity = _extract_pair(identifier_file, object_file) | ||
| results.append((id, entity)) | ||
| for proto_file in filename_iterator: | ||
| # Serialized proto files are of the form: foo.bar.<resource_type>.pb |
There was a problem hiding this comment.
can we add the leading 007_ or whatever to the comment too? the index to keep things in order.
| * Ordered in the order that you want registration to happen. pyflyte should have done the topological sort | ||
| for you and produced file that have a prefix that sets the correct order. | ||
| for you and produced file that have a prefix that sets the correct order.\n |
There was a problem hiding this comment.
do we really need these?
There was a problem hiding this comment.
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?
| @@ -137,22 +138,12 @@ def serialize_all(project, domain, pkgs, version, folder=None): | |||
| continue | |||
| serialized = entity.serialize() | |||
| fname_index = str(i).zfill(zero_padded_length) | |||
| fname = "{}_{}.pb".format(fname_index, entity.id.name) | |||
| click.echo(f" Writing type: {entity.id.resource_type}, {entity.id.name} to\n {fname}") | |||
| fname = "{}_{}.{}.pb".format(fname_index, entity.id.name, entity.id.resource_type_name().lower()) | |||
There was a problem hiding this comment.
so we're embedding the name of the entity into the file name? I wasn't going to do this because of case-sensitivity, os level constraints on file names, length restrictions and the like. Do you really not like the separate file idea?
There was a problem hiding this comment.
from looking online, modern systems allow you up to 255 chars but i can change this to a single char ['t', 'w', 'l'] if theres a concern. the code should handle calling .upper when parsing. i prefer to avoid a separate identifier file when we can be concise with this alternative but if you feel strongly i'm happy to change back.
There was a problem hiding this comment.
talked offline, reverted back to using identifier files
| @@ -33,6 +33,9 @@ def resource_type(self): | |||
| """ | |||
| return self._resource_type | |||
|
|
|||
| def resource_type_name(self) -> str: | |||
There was a problem hiding this comment.
can we add a unit test for this?
There was a problem hiding this comment.
it all comes from generated protobuf code but sure
| @@ -20,16 +20,6 @@ | |||
|
|
|||
|
|
|||
| @click.group("pyflyte", invoke_without_command=True) | |||
| @click.option( | |||
There was a problem hiding this comment.
so this means that if you have pyflyte -p proj -d domain register workflows it'll have to be changed to pyflyte register -p proj -d domain workflows right?
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.
@kumare3 any thoughts on getting rid of pyflyte register altogether?
@wild-endeavor there's also the pyflyte lp command group too
There was a problem hiding this comment.
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.
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
|
PTAL @wild-endeavor |
Will add unit tests if overall this looks okay