Flytekit remote updates for notebooks and versions #3109
Conversation
Signed-off-by: Ketan Umare <[email protected]>
kumare3
requested review from
wild-endeavor,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #7eedadActionable Suggestions - 1
Additional Suggestions - 1
Review Details
AI Code Review powered by |
flytekit/remote/remote.py
Outdated
| def _resolve_version( | ||
| self, version: typing.Optional[str], entity: typing.Any, ss: SerializationSettings | ||
| ) -> typing.Tuple[str, typing.Optional[PickledEntity]]: | ||
| if version is None and self.interactive_mode_enabled: | ||
| md5_bytes, pickled_target_dict = _get_pickled_target_dict(entity) | ||
| return self._version_from_hash( | ||
| md5_bytes, ss, entity.python_interface.default_inputs_as_kwargs, *self._get_image_names(entity) | ||
| ), pickled_target_dict | ||
| elif version is not None: | ||
| return version, None | ||
| raise ValueError( | ||
| "Version must be provided when not in interactive mode. If you want to use latest version pass 'latest'" | ||
| ) |
There was a problem hiding this comment.
Consider extracting the version resolution logic into a separate method for better code organization. The _resolve_version method currently handles both version resolution and pickled entity management, which could be split for better maintainability.
Code suggestion
Check the AI-generated fix before applying
| def _resolve_version( | |
| self, version: typing.Optional[str], entity: typing.Any, ss: SerializationSettings | |
| ) -> typing.Tuple[str, typing.Optional[PickledEntity]]: | |
| if version is None and self.interactive_mode_enabled: | |
| md5_bytes, pickled_target_dict = _get_pickled_target_dict(entity) | |
| return self._version_from_hash( | |
| md5_bytes, ss, entity.python_interface.default_inputs_as_kwargs, *self._get_image_names(entity) | |
| ), pickled_target_dict | |
| elif version is not None: | |
| return version, None | |
| raise ValueError( | |
| "Version must be provided when not in interactive mode. If you want to use latest version pass 'latest'" | |
| ) | |
| def _get_pickled_entity(self, entity: typing.Any) -> typing.Optional[PickledEntity]: | |
| if not self.interactive_mode_enabled: | |
| return None | |
| md5_bytes, pickled_target_dict = _get_pickled_target_dict(entity) | |
| return pickled_target_dict | |
| def _resolve_version( | |
| self, version: typing.Optional[str], entity: typing.Any, ss: SerializationSettings | |
| ) -> typing.Tuple[str, typing.Optional[PickledEntity]]: | |
| if version is not None: | |
| return version, None | |
| if not self.interactive_mode_enabled: | |
| raise ValueError( | |
| "Version must be provided when not in interactive mode. If you want to use latest version pass 'latest'" | |
| ) | |
| md5_bytes, pickled_target_dict = _get_pickled_target_dict(entity) | |
| version = self._version_from_hash( | |
| md5_bytes, ss, entity.python_interface.default_inputs_as_kwargs, *self._get_image_names(entity) | |
| ) | |
| return version, pickled_target_dict |
Code Review Run #7eedad
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: Samhita Alla <[email protected]>
Code Review Agent Run #dda473Actionable Suggestions - 2
Review Details
AI Code Review powered by |
| def _resolve_version( | ||
| self, version: typing.Optional[str], entity: typing.Any, ss: SerializationSettings | ||
| ) -> typing.Tuple[str, typing.Optional[PickledEntity]]: | ||
| if version is None and self.interactive_mode_enabled: | ||
| md5_bytes, pickled_target_dict = _get_pickled_target_dict(entity) | ||
| return self._version_from_hash( | ||
| md5_bytes, ss, entity.python_interface.default_inputs_as_kwargs, *self._get_image_names(entity) | ||
| ), pickled_target_dict | ||
| elif version is not None: | ||
| return version, None | ||
| raise ValueError( | ||
| "Version must be provided when not in interactive mode. If you want to use latest version pass 'latest'" | ||
| ) |
There was a problem hiding this comment.
Consider adding type hints for entity parameter in _resolve_version(). The current implementation accepts typing.Any which could lead to runtime errors if invalid types are passed.
Code suggestion
Check the AI-generated fix before applying
- def _resolve_version(self, version: typing.Optional[str], entity: typing.Any, ss: SerializationSettings
+ def _resolve_version(self, version: typing.Optional[str], entity: typing.Union[WorkflowBase, PythonTask], ss: SerializationSettings
Code Review Run #dda473
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
| project=self.default_project, | ||
| domain=self.default_domain, | ||
| ) | ||
|
|
||
| version, _ = self._resolve_version(version, entity, serialization_settings) |
There was a problem hiding this comment.
Consider moving the version resolution logic before the serialization settings initialization to avoid potential inconsistencies between version and settings.
Code suggestion
Check the AI-generated fix before applying
- serialization_settings = SerializationSettings(
- image_config=ImageConfig.auto_default_image(),
- project=self.default_project,
- domain=self.default_domain,
- )
-
- version, _ = self._resolve_version(version, entity, serialization_settings)
+ version, _ = self._resolve_version(version, entity, None)
+ serialization_settings = SerializationSettings(
+ image_config=ImageConfig.auto_default_image(),
+ project=self.default_project,
+ domain=self.default_domain,
+ version=version
+ )
Code Review Run #dda473
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
|
There's a breaking change here I wanted to highlight which may or may not be okay. I think it's technically more correct, but it is a change in behavior. Assuming you have a simple file like this from flytekit import task, workflow, ImageSpec, FlyteRemote, Config
image = ImageSpec(
name="yt_public",
builder="default",
registry="ghcr.io/wild-endeavor",
packages=["flytekit==v1.15.0b2"],
)
@task(container_image=image, enable_deck=True)
def print_hello():
print("hello")
@workflow
def wf():
print_hello()
if __name__ == "__main__":
remote = FlyteRemote(Config.for_sandbox(), default_project="flytesnacks", default_domain="development")
remote.register_workflow(wf, version="from_main_v4_pr")the behavior on master is as follows
The reason the code is built into the image in the second case is because the code currently is setting the project root. When the register workflow call triggers the downstream register task call, it gets set on the image spec, causing the user code to be built in. With this pr, without setting project root, pyflyte register remains the same, but If the user wants to make this work, they'd have to change to I think we can make a better experience actually - the register_workflow function right now never does fast register... even if the user passes in a serialization settings object with fast register enabled, so there's some discrepancy here to clean up. |
Problems:
latestas a special versionExample cases, now works
Summary by Bito
This PR implements standardized version handling in Flytekit's remote operations, introducing 'latest' as the default version option. The changes include refining the _resolve_version method for consistent version resolution and making version parameters optional with proper defaults. The update improves behavior across interactive and non-interactive modes, particularly enhancing user experience in notebook environments while improving error messaging and documentation.Unit tests added: False
Estimated effort to review (1-5, lower is better): 2