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

Flytekit remote updates for notebooks and versions #3109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kumare3
Copy link
Contributor

@kumare3 kumare3 commented Feb 5, 2025

Problems:

  1. Version had to be provided when registering certain entities from the notebook
  2. Execute worked, but register did not work in the same way
  3. In cases when same entity is registered in non interactive mode, remote entity was auto selected to be latest registered if version field was empty. This is not explicitly called latest as a special version
  4. Makes version handling uniform across all entities

Example cases, now works
Screenshot 2025-02-04 at 9 35 13 PM

Screenshot 2025-02-04 at 9 35 47 PM Screenshot 2025-02-04 at 9 36 12 PM

Summary by Bito

This PR implements standardized version handling in Flytekit's remote operations, introducing 'latest' as the default version option. The changes include a new _resolve_version method for consistent version resolution and improved registration process across interactive and non-interactive modes. The update enhances error messaging and version handling documentation.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

@flyte-bot
Copy link
Contributor

flyte-bot commented Feb 5, 2025

Code Review Agent Run #7eedad

Actionable Suggestions - 1
  • flytekit/remote/remote.py - 1
Additional Suggestions - 1
  • flytekit/remote/remote.py - 1
    • Consider consolidating version check conditions · Line 166-167
Review Details
  • Files reviewed - 1 · Commit Range: 9e0af8a..9e0af8a
    • flytekit/remote/remote.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Contributor

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Enhanced Version Handling in Remote Operations

remote.py - Improved version handling logic with 'latest' as default, added version resolution method, and updated entity registration behavior

Comment on lines +2055 to +2067
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'"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider splitting version resolution logic

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
Suggested change
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants