Feat!: Remove SDK installation from our base image #443

SebastianMorawiec · 2024-08-23T10:24:59Z

The problem

We had to regenerate SDK images each release
https://zapatacomputing.atlassian.net/browse/ORQSDK-1032

This PR's solution

Remove installation of SDK in our worker base images

Checklist

Check that this PR satisfies the following items:

Tests have been added for new features/changed behavior (if no new features have been added, check the box).
The changelog file has been updated with a user-readable description of the changes (if the change isn't visible to the user in any way, check the box).
The PR's title is prefixed with <feat/fix/chore/imp[rovement]/int[ernal]/docs>[!]:
The PR is linked to a JIRA ticket (if there's no suitable ticket, check the box).

github-actions · 2024-08-23T10:25:51Z

🚀 Code Coverage

---------------------------------------------------------------------------------------
src/orquestra/workflow_runtime/_ray/_build_workflow.py      258     45    83%   65-66, 171-177, 189-190, 223, 437-451, 473-479, 492, 563-576, 585-589, 656-663, 696-698
src/orquestra/workflow_runtime/_ray/_client.py               90     12    87%   27-33, 172-173, 187-188, 210-211
src/orquestra/workflow_runtime/_ray/_dag.py                 218     21    90%   81, 241, 351-366, 425-426, 533-534, 554-557, 583-585, 660-662, 698-699, 783, 794
src/orquestra/workflow_runtime/_ray/_dirs.py                 24      8    67%   29-32, 37-40
src/orquestra/workflow_runtime/_testing/_connections.py      38      6    84%   34, 43-56, 83-85
---------------------------------------------------------------------------------------
TOTAL                                                       853     92    89%

6 files skipped due to complete coverage.
-------------
Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes
-------------
projects/orquestra-workflow-runtime/src/orquestra/workflow_runtime/_ray/_build_workflow.py (91.7%): Missing lines 585
-------------
Total:   12 lines
Missing: 1 line
Coverage: 91%
-------------

alexjuda

Found few blocking issues, the all the nitpicks are non-blocking

alexjuda · 2024-09-02T13:09:32Z

projects/orquestra-sdk/tests/integration/ray/test_integration.py

-                # We should only have two calls: our invocation and the aggregation step
-                assert len(calls) == 2
+                # We should only have three calls:
+                # invocation aggregation step and error handling for aggregation step


What's the 3rd call?

alexjuda · 2024-09-16T12:52:15Z

projects/orquestra-workflow-runtime/src/orquestra/workflow_runtime/_ray/_build_workflow.py

@@ -403,7 +393,13 @@ def _(imp: ir.GitImport):
        "" if imp.package_name is None else f"{imp.package_name}{extras_string} @ "
    )

-    return [f"{package_name_string}{url_string}"]
+    # TODO: we should fully support subdirectories as projects


nitpick TODO without a ticket

alexjuda · 2024-09-16T12:55:08Z

projects/orquestra-sdk/docker/Dockerfile

@@ -38,7 +37,7 @@ RUN <<EOF
 set -ex
 . "$VIRTUAL_ENV/bin/activate"
 python -m pip install --no-cache-dir -U pip wheel
-python -m pip install --no-cache-dir "${SDK_REQUIREMENT}"
+python -m pip install --no-cache-dir ray[default]==2.30


question: I can imagine the Ray version we're using in the image has to match the Ray version used in other components. Can you remind me what those components are?

It would be great to make a list of them here as a comment, so we know what to pay attention to when bumping the constraint in the future.

Ray version is important in 2 places

wdr

image being run

B\umping ray version would be the only scenario where we have to rebuild all custom-images as they are based on given ray version.

It will be part of release process

alexjuda · 2024-09-16T15:29:26Z

projects/orquestra-sdk/docs/examples/tests/test_dependency_installation.py

-        assert deps_imports[0].type == "PYTHON_IMPORT"
+        # it depends if you have installed SDK with version from PyPi, or
+        # its local-dev version, it changes what import is used for SDK
+        assert deps_imports[0].type == "PYTHON_IMPORT" or "GIT_IMPORT"


issue: that's not how equality works in Python 😄

>>> foo = "foo" >>> foo == "abc" or "def" 'def'

Suggested change

assert deps_imports[0].type == "PYTHON_IMPORT" or "GIT_IMPORT"

assert deps_imports[0].type in {"PYTHON_IMPORT", "GIT_IMPORT"}

alexjuda · 2024-09-16T15:35:38Z

projects/orquestra-sdk/src/orquestra/sdk/_client/_base/_docker_images.py

issue: the design doc mentions specifying the images in orquestra-workflow-shared, while this is in orquestra-sdk. Which approach is the intended one?

Good catch - I fixed the design doc.

Reasoning:
WDR is dependent on runtime-shared.
If I update WDR to new version, I need to update shared library.
If I updated shared library, I would have to update WDR.
This would basically lead to the looped process.

Now when we update WDR we update client and everything works.

alexjuda · 2024-09-16T15:40:46Z

projects/orquestra-sdk/tests/sdk/test_traversal.py

@@ -1178,6 +1190,7 @@ def test_metadata_on_dev(monkeypatch: pytest.MonkeyPatch):
    # Given
    mocked_installed_version = Mock(return_value="22.42.0.dev1+gitAABBCC.20230101")
    monkeypatch.setattr(_versions, "get_installed_version", mocked_installed_version)
+    monkeypatch.setattr(git.Repo, "is_dirty", Mock(return_value=False))


nitpick: this mock is already set by the autoused fixture mask_sdk_version

Suggested change

monkeypatch.setattr(git.Repo, "is_dirty", Mock(return_value=False))

projects/orquestra-workflow-runtime/tests/data/.gitignore

alexjuda · 2024-09-16T15:43:22Z

projects/orquestra-workflow-shared/src/orquestra/workflow_shared/schema/ir.py

@@ -384,6 +384,8 @@ class Version(BaseModel):
 class WorkflowMetadata(BaseModel):
    sdk_version: Version
    python_version: Version
+    # new field added in 0.67. Default to None to allow parsing older IRs


nice addition and a nice comment 👍

SebastianMorawiec added 4 commits August 22, 2024 13:15

remove SDK from base image

a02a7c5

change default image used

35382ab

fix cuda dockerfile

005edf7

remove default template

fe77b8b

SebastianMorawiec requested a review from a team August 23, 2024 10:24

SebastianMorawiec added 2 commits August 23, 2024 12:33

fix style

caa942d

fix integration tests

9f15ea8

SebastianMorawiec marked this pull request as draft August 23, 2024 11:07

SebastianMorawiec added 21 commits August 26, 2024 13:09

use custom WDR image

580c25e

pass without sdk

f7f78f9

remove another place with sdk info

a3b748d

change to staging nexus

bac9598

Add pip install

d6ee436

fix args

d7ac9a4

name change

45138e6

pip

1c9128b

temp stuff

88559c1

add sdk everywhere

7f76896

try once again

2290414

add runtime everywhere

3f629c8

fix data aggregation step

0c65a89

install minimum stuff

5d2739b

redo how we read sdk version installation in worker

dbe0d47

Merge remote-tracking branch 'origin/main' into Morawiec/sdk_base

d3e2cb9

remove fitering out and warning on server side

1db2516

fix data aggregation step

a33f4bc

fixes

8ee94ce

head_node_image has to be backwards-compatible

c4447b0

add custom image on client side

02a6bc8

SebastianMorawiec added 19 commits August 29, 2024 13:18

cleanup IR

d332f4c

remove bad code

5ba2dbb

fix installation of workflow runtime

6d508d4

fix sdk fast tests

a92fa30

add gitignore for generated jsons for tests

dd5f446

satisfy pyright

fcced82

add comment

eee917a

fix handling dev environment

e36e25b

fix style

a350529

filter warnings

801b8f9

add repo download to tests

71e5831

change url

64d2208

change to github import

f0f2fec

add subdirectory

8771762

remove stubbed version

1eb9545

fix style

c8cb657

try fixing tests

c25d9df

style

002ca72

fix integration tests

38d3edd

SebastianMorawiec marked this pull request as ready for review August 30, 2024 14:57

SebastianMorawiec added 2 commits September 2, 2024 13:48

fix integration tests

06fbecf

fix tests

8ed835f

alexjuda suggested changes Sep 16, 2024

View reviewed changes

SebastianMorawiec added 3 commits September 24, 2024 13:32

Merge remote-tracking branch 'origin/main' into Morawiec/sdk_base

cef8c4b

review adjustments

0e9ab97

fix style

83fb603

alexjuda approved these changes Sep 25, 2024

View reviewed changes

SebastianMorawiec merged commit 26daea6 into main Sep 25, 2024
21 of 22 checks passed

SebastianMorawiec deleted the Morawiec/sdk_base branch September 25, 2024 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat!: Remove SDK installation from our base image #443

Feat!: Remove SDK installation from our base image #443

SebastianMorawiec commented Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 23, 2024 •

edited

Loading

alexjuda left a comment

alexjuda Sep 2, 2024

alexjuda Sep 16, 2024

alexjuda Sep 16, 2024

SebastianMorawiec Sep 25, 2024 •

edited

Loading

alexjuda Sep 16, 2024

SebastianMorawiec Sep 25, 2024

alexjuda Sep 16, 2024

SebastianMorawiec Sep 25, 2024

alexjuda Sep 16, 2024

alexjuda Sep 16, 2024

	assert deps_imports[0].type == "PYTHON_IMPORT" or "GIT_IMPORT"
	assert deps_imports[0].type in {"PYTHON_IMPORT", "GIT_IMPORT"}

Feat!: Remove SDK installation from our base image #443

Feat!: Remove SDK installation from our base image #443

Conversation

SebastianMorawiec commented Aug 23, 2024 • edited Loading

The problem

This PR's solution

Checklist

github-actions bot commented Aug 23, 2024 • edited Loading

alexjuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMorawiec Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianMorawiec commented Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 23, 2024 •

edited

Loading

SebastianMorawiec Sep 25, 2024 •

edited

Loading