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

Remove mount usage in tests #2766

Merged
merged 8 commits into from
Jan 17, 2025
Merged

Conversation

freider
Copy link
Contributor

@freider freider commented Jan 16, 2025

Replaces Mount.from_local usages in tests to not trigger deprecation warnings.

Three different ways of removing:

  • Some are kept around temporarily using the new private equivalents Mount._from_local*, to prevent regressions in the mount=-SDK surfaces until we fully deprecate these methods.
  • Some are replaced with Image.add_local equivalents
  • Some docstring code snippets are kept as is with a deprecation note in the docstring and notest added to the fence

Check these boxes or delete any item (or this section) if not relevant for this PR.

  • Client+Server: this change is compatible with old servers
  • Client forward compatibility: this change ensures client can accept data intended for later versions of itself

Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.


Changelog

Copy link
Contributor

@mwaskom mwaskom left a comment

Choose a reason for hiding this comment

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

modal.Image.debian_slim()
.add_local_python_source("pkg_a") # identical to first explicit mount and auto mounts
.add_local_python_source(
# custom ignore condition, include normally_not_included.pyc (but skip __pycache__)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about how this example is including .pyc files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry this was really stupid in hindsight - the test creates a .pyc-file outside of the __pycache__ dir, but which is still ignored by default. In the second condition we only ignore the __pycache__ dir content and as such the .pyc file is included 😬

@@ -221,16 +221,10 @@ def test_image_kwargs_validation(builder_version, servicer, client):
secrets=[
Secret.from_dict({"xyz": "123"}),
Secret.from_name("foo"),
Mount.from_local_dir("/", remote_path="/"), # type: ignore
Dict.from_name("mydict"), # type: ignore
], # Mount is not a valid Secret
Copy link
Contributor

Choose a reason for hiding this comment

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

Update comment too?

@freider freider merged commit 653aa96 into main Jan 17, 2025
23 checks passed
@freider freider deleted the freider/remove-mount-usage-in-tests branch January 17, 2025 13:20
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