Skip to content

fix: add task config to version hash calculation#2417

Closed
ddl-rliu wants to merge 2 commits into
flyteorg:masterfrom
dominodatalab:rliu.add-task-config-to-version-hash
Closed

fix: add task config to version hash calculation#2417
ddl-rliu wants to merge 2 commits into
flyteorg:masterfrom
dominodatalab:rliu.add-task-config-to-version-hash

Conversation

@ddl-rliu

@ddl-rliu ddl-rliu commented May 14, 2024

Copy link
Copy Markdown
Contributor

Tracking issue

Closes flyteorg/flyte#5364

Why are the changes needed?

Failed to register a workflow if users update the task config – especially in the case of custom tasks

What changes were proposed in this pull request?

hash should be calculated from the task config

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Tangentially related PRs:
flyteorg/flyte#4904
#2356
#2120

Docs link

@ddl-rliu ddl-rliu changed the title [BUG] Task config should be an input into computing the task version for fast registration fix: add task config to version hash calculation May 14, 2024
@ddl-ebrown

Copy link
Copy Markdown
Contributor

FYI there's a failing test b/c the mocked call isn't expecting the new param

=========================== short test summary info ============================
FAILED tests/flytekit/unit/remote/test_remote.py::test_get_image_names - AssertionError: expected call not found.
Expected: _version_from_hash(b'\x01\x02\x03', <ANY>, <ANY>, 'flyteorg/flytekit:5QQUz7hCXJyIkYOg6eFl0w')
Actual: _version_from_hash(b'\x01\x02\x03', SerializationSettings(image_config=ImageConfig(default_image=Image(name='default', fqn='cr.flyte.org/flyteorg/flytekit', tag='py3.12-latest', digest=None), images=[Image(name='default', fqn='cr.flyte.org/flyteorg/flytekit', tag='py3.12-latest', digest=None)]), project=None, domain=None, version=None, env=None, git_repo='', python_interpreter='/opt/venv/bin/python3', flytekit_virtualenv_root='/opt/venv', fast_serialization_settings=FastSerializationSettings(enabled=True, destination_dir='.', distribution_location='localhost:30084'), source_root=None), {'name': 'union'}, 'flyteorg/flytekit:5QQUz7hCXJyIkYOg6eFl0w', 'None')
====== 1 failed, 926 passed, 62 skipped, 690 warnings in 73.57s (0:01:13) ======

Basically the line at

version_from_hash_mock.assert_called_once_with(md5_bytes, mock.ANY, mock.ANY, image_spec.image_name())
needs to be updated for the None at the end

@ddl-rliu ddl-rliu force-pushed the rliu.add-task-config-to-version-hash branch from e633838 to 4f3e49f Compare May 15, 2024 19:47
LegacyConfigEntry(SECTION, "ca_cert_file_path"), YamlConfigEntry("admin.caCertFilePath")
)
HTTP_PROXY_URL = ConfigEntry(LegacyConfigEntry(SECTION, "http_proxy_url"), YamlConfigEntry("admin.httpProxyURL"))
VERSION_HASH_SKIP_TASK_CONFIG = ConfigEntry(LegacyConfigEntry(SECTION, "version_hash_skip_task_config"), YamlConfigEntry("admin.versionHashSkipTaskConfig", bool))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it preferred to gate this behavior behind config? If so, it likely doesn't belong in the PlatformConfig section – happy to move it somewhere else.

ddl-rliu added 2 commits May 15, 2024 13:41
x
Signed-off-by: ddl-rliu <richard.liu@dominodatalab.com>
Signed-off-by: ddl-rliu <richard.liu@dominodatalab.com>
@ddl-rliu ddl-rliu force-pushed the rliu.add-task-config-to-version-hash branch from 4f3e49f to 3ff0b62 Compare May 15, 2024 20:41
@ddl-rliu

Copy link
Copy Markdown
Contributor Author

Closing in favor of #2428

@ddl-rliu ddl-rliu closed this May 22, 2024
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.

[BUG] Task config should be used when computing the task version hash

2 participants