Annotations separate out serializing #326
Conversation
wild-endeavor
requested review from
kumare3 and
matthewphsmith
as code owners
flytekit/common/translator.py
Outdated
|
|
||
|
|
||
| def get_serializable( | ||
| settings: RegistrationSettings, entity: FlyteLocalEntity, fast: Optional[bool] = False |
There was a problem hiding this comment.
what are your thoughts on renaming RegistrationSettings to SerializationSettings since that's now the primary use-case?
flytekit/common/translator.py
Outdated
| if fast: | ||
| # Containerless tasks are always fast registerable without modification so only operate on tasks that | ||
| # have a container. Also, raw ContainerTask's should never be touched. | ||
| if cp_entity.container is not None and not isinstance(entity, ContainerTask): |
There was a problem hiding this comment.
hm it makes me sad we lose the inheritence bit. i wonder if it's worth refactoring get_container to accept a bool for whether the context should be regular or fast
There was a problem hiding this comment.
interesting, also, maybe we should do this for any entity that is instance of PythonAutoContainerTask and not ContainerTask. We should assume all container tasks are independent of code?
There was a problem hiding this comment.
Can we do this in a later PR? As indicated by Miguel's recent question, I want to see if there are additional use cases for changing/overriding the command of a container. Even if not, I'd rather clean up in a separate PR since it's not user facing.
|
|
||
|
|
||
| # TODO we should accept TaskMetadata here and then extract whatever fields we want into NodeMetadata | ||
| def create_and_link_node( |
There was a problem hiding this comment.
maybe the name of this function needs to be renamed to reflect that we are Substituting outputs with promises
| cp_entity.assign_name(entity.reference.id.name) | ||
|
|
||
| elif isinstance(entity, ReferenceWorkflow): | ||
| workflow_metadata = WorkflowMetadata(on_failure=WorkflowFailurePolicy.FAIL_IMMEDIATELY) |
There was a problem hiding this comment.
Why?
Why does a reference workflow not look similar, maybe i am missing something
There was a problem hiding this comment.
what do you mean? similar to what?
|
This looks aaamazing! awesome work |
flytekit/common/translator.py
Outdated
| ) | ||
|
|
||
| elif isinstance(entity, ReferenceLaunchPlan): | ||
| wf_id = _identifier_model.Identifier(_identifier_model.ResourceType.WORKFLOW, "", "", "", "") |
There was a problem hiding this comment.
not related to this PR but why do you use an empty identiifer rather than filling out the fields?
There was a problem hiding this comment.
No intelligent reason
There was a problem hiding this comment.
this will all get deleted i hope
NodeOutputclass in thepromisefile not dependent on the old one. I left an unusedsdk_typein there for now cuz that's always None but I'll change it.create_and_link_nodeto thepromisefile.get_registerable_entitylogic from everything and put all the logic in one file.RegistrationSettingsobject inside a context object because the new function just expects it explicitly. However I didn't see an easy way to remove it from theFlyteContextentirely because of dynamic tasks, which run a compilation step inside of execution and registration settings are not usually passed through during execution.