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

soopervisor --skip-docker should use the configured repository when running with AWS Batch #132

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

meprem
Copy link

@meprem meprem commented Jan 24, 2025

Describe your changes

Right now, when using --skip-docker, the Batch exporter tries to determine the name of the container for the Job definition based on the package the soopervisor.yml file is in, but ideally it'd reuse the configured repository within that file in the Job definition if it's already provided.

Issue ticket number and link

Closes #130 #130

Checklist before requesting a review

  • [ *] I have performed a self-review of my code
  • I have added thorough tests (when necessary).
  • I have added the right documentation (when needed). Product update? If yes, write one line about this update.

Tested in AWS Batch, earlier it was submitting task using invalid image. Now, it's able to re-use previously created docker image.

 $aws ecr get-login-password --region us-west-2 | docker login --username AWS --password-stdin xxx.dkr.ecr.us-west-2.amazonaws.com/prod-repo && soopervisor export deploy --skip-tests --ignore-git --skip-docker

…unning with AWS Batch

Fixed ploomber#130

Tested in AWS Batch, earlier it was submitting task using invalid image. Now, it's able to re-use previously created docker image.

```
 $aws ecr get-login-password --region us-west-2 | docker login --username AWS --password-stdin xxx.dkr.ecr.us-west-2.amazonaws.com/prod-repo && soopervisor export deploy --skip-tests --ignore-git --skip-docker
```
soopervisor --skip-docker should use the configured repository when r…
@edublancas
Copy link
Contributor

can you fix the CI @meprem? I think the problem is that we're using an old Python version, I'm guessing upgrading to 3.10 or higher will fix the issue

@meprem
Copy link
Author

meprem commented Jan 30, 2025

@edublancas I will take a look... thank you.

Copy link
Author

@meprem meprem left a comment

Choose a reason for hiding this comment

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

Tested it locally.

(soopervisor) premkumar@Prems-MacBook-Pro soopervisor % invoke test
...
tests/test_config_classes.py::test_schema[KubeflowConfig-initial5-schema5]
  /Users/premkumar/Downloads/code/soopervisor/tests/test_config_classes.py:122: PydanticDeprecatedSince20: The `dict` method is deprecated; use `model_dump` instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.10/migration/
    assert class_(**initial).dict() == schema

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================= 282 passed, 12 skipped, 2 xfailed, 200334 warnings in 186.25s (0:03:06) ==================================================================
Traceback (most recent call last):
  File "/Users/premkumar/miniconda3/envs/soopervisor/lib/python3.9/multiprocessing/resource_tracker.py", line 201, in main
    cache[rtype].remove(name)
KeyError: '/loky-26148-9pgu6vbm'
(soopervisor) premkumar@Prems-MacBook-Pro soopervisor %

I don't think we need the following change in the pipeline:

(soopervisor) premkumar@Prems-MacBook-Pro soopervisor % git diff
diff --git a/tests/test_commons_docker.py b/tests/test_commons_docker.py
index ee2484f..63cd0ff 100644
--- a/tests/test_commons_docker.py
+++ b/tests/test_commons_docker.py
@@ -223,7 +223,7 @@ def test_docker_build(EXPORTER, config, tmp_fast_pipeline, capfd, monkeypatch):
     assert (
         env_contents
         == """\
-git: master
+git: main
 git_hash: SOMEHASH
 """
     )
@@ -238,17 +238,17 @@ git_hash: SOMEHASH
         [
             None,
             None,
-            {"git": "master", "git_hash": "SOMEHASH"},
+            {"git": "main", "git_hash": "SOMEHASH"},
         ],
         [
             "env.something.yaml",
             {"some": {"nested": "value"}},
-            {"git": "master", "git_hash": "SOMEHASH", "some": {"nested": "value"}},
+            {"git": "main", "git_hash": "SOMEHASH", "some": {"nested": "value"}},
         ],
         [
             None,
             {"some": {"nested": "value"}},
-            {"git": "master", "git_hash": "SOMEHASH", "some": {"nested": "value"}},
+            {"git": "main", "git_hash": "SOMEHASH", "some": {"nested": "value"}},
         ],
     ],
     ids=[
(soopervisor) premkumar@Prems-MacBook-Pro soopervisor %

@meprem
Copy link
Author

meprem commented Feb 17, 2025

I was able to reproduce the issue on ubuntu, and after upgrading from 3.8 to 3.9, tests were successful.

(soopervisor) prem@prem-7960:~/Downloads/soopervisor$ invoke test
...
tests/test_config_classes.py::test_schema[AWSLambdaConfig-initial4-schema4]
tests/test_config_classes.py::test_schema[KubeflowConfig-initial5-schema5]
  /home/prem/Downloads/soopervisor/tests/test_config_classes.py:122: PydanticDeprecatedSince20: The `dict` method is deprecated; use `model_dump` instead. Deprecated in Pydantic V2.0 to be removed in V3.0. See Pydantic V2 Migration Guide at https://errors.pydantic.dev/2.10/migration/
    assert class_(**initial).dict() == schema

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================== 283 passed, 12 skipped, 1 xfailed, 200334 warnings in 223.28s (0:03:43) ==============================================
(soopervisor) prem@prem-7960:~/Downloads/soopervisor$ 

@meprem
Copy link
Author

meprem commented Feb 18, 2025

@edublancas can you please review and merge these changes? Thank you.

@edublancas
Copy link
Contributor

I see the CI is failing, my guess is that the python version is still the culprit - perhaps try 3.11 or 3.12

@meprem
Copy link
Author

meprem commented Feb 26, 2025

I see the CI is failing, my guess is that the python version is still the culprit - perhaps try 3.11 or 3.12

Ahh... Let me try upgrading the version.

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.

soopervisor --skip-docker should use the configured repository when running with AWS Batch
2 participants