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

fix(cloud): detect and ignore venv #16056

Merged
merged 6 commits into from
Dec 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/lightning_app/runners/cloud.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,14 @@ def dispatch(
else:
ignore_functions = None

# Create a default dotignore if it doesn't exist
if not (root / DOT_IGNORE_FILENAME).is_file():
with open(root / DOT_IGNORE_FILENAME, "w") as f:
f.write("venv/\n")
if (root / "bin" / "activate").is_file() or (root / "pyvenv.cfg").is_file():
Copy link
Contributor

Choose a reason for hiding this comment

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

We could write the lines below without checking this. Do you expect any impact of having extra ignores?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check checks whether we are inside of venv and then ignores typical directories that exist in a venv dir.

Copy link
Contributor

@carmocca carmocca Dec 15, 2022

Choose a reason for hiding this comment

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

I get that. I meant if we should ignore them even if we aren't inside a venev.
This is generally convenient. For example, just as GitHub creates a .gitignore for you with a bunch of Python blobs from libraries even if you don't use any of them.

Do you expect any harm in that?

# the user is developing inside venv
f.write("bin/\ninclude/\nlib/\npyvenv.cfg\n")

repo = LocalSourceCodeDir(path=root, ignore_functions=ignore_functions)
self._check_uploaded_folder(root, repo)
requirements_file = root / "requirements.txt"
Expand Down Expand Up @@ -556,15 +564,10 @@ def _check_uploaded_folder(root: Path, repo: LocalSourceCodeDir) -> None:
f"Your application folder '{root.absolute()}' is more than {CLOUD_UPLOAD_WARNING} MB. "
f"The total size is {round(app_folder_size_in_mb, 2)} MB. {len(files)} files were uploaded.\n"
+ largest_paths_msg
+ "Perhaps you should try running the app in an empty directory."
+ "Perhaps you should try running the app in an empty directory.\n"
+ "You can ignore some files or folders by adding them to `.lightningignore`.\n"
+ " You can also set the `self.lightningingore` attribute in a Flow or Work."
)
if not (root / DOT_IGNORE_FILENAME).is_file():
warning_msg += (
"\nIn order to ignore some files or folder, create a `.lightningignore` file and add the paths to"
" ignore. You can also set the `lightningingore` attribute in a Flow or Work."
)
else:
warning_msg += "\nYou can ignore some files or folders by adding them to `.lightningignore`."

logger.warn(warning_msg)

Expand Down
52 changes: 51 additions & 1 deletion tests/tests_app/runners/test_cloud.py
Original file line number Diff line number Diff line change
Expand Up @@ -1334,7 +1334,7 @@ def test_check_uploaded_folder(monkeypatch, tmpdir, caplog):
assert "The total size is 15.0 MB" in caplog.text
assert "3 files were uploaded" in caplog.text
assert "files:\n6.0 MB: c.jpg\n5.0 MB: b.txt\n4.0 MB: a.png\nPerhaps" in caplog.text # tests the order
assert "create a `.lightningignore` file" in caplog.text
assert "adding them to `.lightningignore`." in caplog.text
yurijmikhalevich marked this conversation as resolved.
Show resolved Hide resolved
assert "lightningingore` attribute in a Flow or Work" in caplog.text


Expand Down Expand Up @@ -1571,6 +1571,56 @@ def run(self):
flow.run()


def test_default_lightningignore(monkeypatch, caplog, tmpdir):
yurijmikhalevich marked this conversation as resolved.
Show resolved Hide resolved
mock_client = mock.MagicMock()
mock_client.projects_service_list_memberships.return_value = V1ListMembershipsResponse(
memberships=[V1Membership(name="test-project", project_id="test-project-id")]
)
mock_client.lightningapp_instance_service_list_lightningapp_instances.return_value = (
V1ListLightningappInstancesResponse(lightningapps=[])
)
mock_client.lightningapp_v2_service_create_lightningapp_release.return_value = V1LightningappRelease(
cluster_id="test"
)
cloud_backend = mock.MagicMock(client=mock_client)
monkeypatch.setattr(backends, "CloudBackend", mock.MagicMock(return_value=cloud_backend))

class MyWork(LightningWork):
def run(self):
pass

app = LightningApp(MyWork())

path = Path(tmpdir)
cloud_runtime = cloud.CloudRuntime(app=app, entrypoint_file=path / "entrypoint.py")
monkeypatch.setattr(LocalSourceCodeDir, "upload", mock.MagicMock())

# write some files
write_file_of_size(path / "a.txt", 5 * 1000 * 1000)
write_file_of_size(path / "venv" / "foo.txt", 4 * 1000 * 1000)

assert not (path / ".lightningignore").exists()

with mock.patch(
"lightning_app.runners.cloud._parse_lightningignore", wraps=_parse_lightningignore
) as parse_mock, mock.patch(
"lightning_app.source_code.local._copytree", wraps=_copytree
) as copy_mock, caplog.at_level(
logging.WARN
):
cloud_runtime.dispatch()

parse_mock.assert_called_once_with(())
assert copy_mock.mock_calls[0].kwargs["ignore_functions"][0].args[1] == set()

assert (path / ".lightningignore").exists()
yurijmikhalevich marked this conversation as resolved.
Show resolved Hide resolved

assert f"Your application folder '{path.absolute()}' is more than 2 MB" in caplog.text
assert "The total size is 5.0 MB" in caplog.text
assert "2 files were uploaded" # a.txt and .lightningignore
assert "files:\n5.0 MB: a.txt\nPerhaps" in caplog.text # only this file appears


@pytest.mark.parametrize(
"lightning_app_instance, lightning_cloud_url, expected_url",
[
Expand Down