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.

Sign up for GitHub

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

[App] Change app root / config path to be the app.py parent directory #15654

Merged
merged 7 commits into from
Nov 11, 2022
Merged

[App] Change app root / config path to be the app.py parent directory #15654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
804f7fb
Change app root / config path to be the `app.py` parent directory
ethanwharris Nov 11, 2022
ca08a14
Update CHANGELOG.md
ethanwharris Nov 11, 2022
0fe2e0d
mypy
ethanwharris Nov 11, 2022
cc1903b
Merge branch 'master' into bugfix/app_root
ethanwharris Nov 11, 2022
bc81cf2
Fix
ethanwharris Nov 11, 2022
1ad15f3
Merge branch 'bugfix/app_root' of https://github.com/Lightning-AI/lig…
ethanwharris Nov 11, 2022
1ea24ca
Mypy
ethanwharris Nov 11, 2022
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
2 changes: 2 additions & 0 deletions src/lightning_app/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
- The `params` argument in `TracerPythonScript.run` no longer prepends `--` automatically to parameters ([#15518](https://github.com/Lightning-AI/lightning/pull/15518))


- Changed the root directory of the app (which gets uploaded) to be the folder containing the app file, rather than any parent folder containing a `.lightning` file ([#15654](https://github.com/Lightning-AI/lightning/pull/15654))

Comment on lines +40 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

This was intentional, see the test cases. The way this PR defines the root is extremely limiting. If the root folder is defined by the app.py file, then your app.py file ALWAYS has to be at the root of your project (the folder requiring all sources to run the program).

We created the .lightning file for two purposes:

  • remember the name of the app
  • define the root of the project (the files that need to be uploaded)

See the specification here: https://www.notion.so/lightningai/Lightning-Dotfile-and-the-cloud-experience-cfe28f6936a642b08958cf86b458cafc#9e47b6ae4b6b48a0b9840887994353b3

How will you address the newly introduced limitations?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would argue a few points:

  • requiring app.py to be at the root is no less flexible than requiring the user to create a .lightning file at the root
  • this behaviour isn't actually documented anywhere and has led to users getting very large directories uploaded on multiple occasions
  • .lightning files are not currently expected to be commited with source files as they also contain the cluster ID - with that new contract, the majority of apps are required to have app.py at the route anyway

Overall I think this approach is much closer to what you expect to happen as a user without having to learn a load about the undocumented internals.

Copy link
Contributor

Choose a reason for hiding this comment

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

The user does not create it manually. It gets generated for them. If the file is not found, it creates it next to the app.py file (documented in the notion page). This means the default experience is good: Only the files next to the app.py file gets uploaded.

I am sorry that there is misinformation around this.

.lightning files are not currently expected to be commited with source files as they also contain the cluster ID - with that new contract, the majority of apps are required to have app.py at the route anyway

Let's document it then? When we built the initial version of the framework, we had no time for anything.


### Deprecated

Expand Down
8 changes: 4 additions & 4 deletions src/lightning_app/runners/cloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
from lightning_app.utilities.cloud import _get_project
from lightning_app.utilities.dependency_caching import get_hash
from lightning_app.utilities.load_app import _prettifiy_exception, load_app_from_file
from lightning_app.utilities.packaging.app_config import AppConfig, find_config_file
from lightning_app.utilities.packaging.app_config import _get_config_file, AppConfig
from lightning_app.utilities.packaging.lightning_utils import _prepare_lightning_wheels_and_requirements
from lightning_app.utilities.secrets import _names_to_ids

Expand Down Expand Up @@ -95,9 +95,9 @@ def dispatch(

# TODO: verify lightning version
# _verify_lightning_version()
config_file = find_config_file(self.entrypoint_file)
app_config = AppConfig.load_from_file(config_file) if config_file else AppConfig()
root = config_file.parent if config_file else Path(self.entrypoint_file).absolute().parent
config_file = _get_config_file(self.entrypoint_file)
app_config = AppConfig.load_from_file(config_file) if config_file.exists() else AppConfig()
root = Path(self.entrypoint_file).absolute().parent
cleanup_handle = _prepare_lightning_wheels_and_requirements(root)
self.app._update_index_file()
repo = LocalSourceCodeDir(path=root)
Expand Down
18 changes: 5 additions & 13 deletions src/lightning_app/utilities/packaging/app_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def save_to_file(self, path: Union[str, pathlib.Path]) -> None:

def save_to_dir(self, directory: Union[str, pathlib.Path]) -> None:
"""Save the configuration to a file '.lightning' to the given folder in YAML format."""
self.save_to_file(pathlib.Path(directory, _APP_CONFIG_FILENAME))
self.save_to_file(_get_config_file(directory))

@classmethod
def load_from_file(cls, path: Union[str, pathlib.Path]) -> "AppConfig":
Expand All @@ -47,22 +47,14 @@ def load_from_dir(cls, directory: Union[str, pathlib.Path]) -> "AppConfig":
return cls.load_from_file(pathlib.Path(directory, _APP_CONFIG_FILENAME))


def find_config_file(source_path: pathlib.Path = pathlib.Path.cwd()) -> Optional[pathlib.Path]:
"""Search for the Lightning app config file '.lightning' at the given source path.

Relative to the given path, it will search for the '.lightning' config file by going up the directory structure
until found. Returns ``None`` if no config file is found in any of the parent directories.
def _get_config_file(source_path: Union[str, pathlib.Path]) -> pathlib.Path:
"""Get the Lightning app config file '.lightning' at the given source path.

Args:
source_path: A path to a folder or a file. The search for the config file will start relative to this path.
source_path: A path to a folder or a file.
"""
source_path = pathlib.Path(source_path).absolute()
if source_path.is_file():
source_path = source_path.parent

candidate = pathlib.Path(source_path / _APP_CONFIG_FILENAME)
if candidate.is_file():
return candidate

if source_path.parents:
return find_config_file(source_path.parent)
return pathlib.Path(source_path / _APP_CONFIG_FILENAME)
22 changes: 5 additions & 17 deletions tests/tests_app/utilities/packaging/test_app_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import pathlib

from lightning_app.utilities.packaging.app_config import AppConfig, find_config_file
from lightning_app.utilities.packaging.app_config import _get_config_file, AppConfig


def _make_empty_config_file(folder):
Expand All @@ -10,24 +10,12 @@ def _make_empty_config_file(folder):
return file


def test_find_config_file(tmpdir, monkeypatch):
monkeypatch.chdir(pathlib.Path("/"))
assert find_config_file() is None

monkeypatch.chdir(pathlib.Path.home())
assert find_config_file() is None

def test_get_config_file(tmpdir):
_ = _make_empty_config_file(tmpdir)
config_file1 = _make_empty_config_file(tmpdir / "a" / "b")

assert find_config_file(tmpdir) == pathlib.Path(tmpdir, ".lightning")
assert find_config_file(config_file1) == pathlib.Path(tmpdir, "a", "b", ".lightning")
assert find_config_file(pathlib.Path(tmpdir, "a")) == pathlib.Path(tmpdir, ".lightning")
config_file1 = _make_empty_config_file(tmpdir)

# the config must be a file, a folder of the same name gets ignored
fake_config_folder = pathlib.Path(tmpdir, "fake", ".lightning")
fake_config_folder.mkdir(parents=True)
assert find_config_file(tmpdir) == pathlib.Path(tmpdir, ".lightning")
assert _get_config_file(tmpdir) == pathlib.Path(tmpdir, ".lightning")
assert _get_config_file(config_file1) == pathlib.Path(tmpdir, ".lightning")


def test_app_config_save_load(tmpdir):
Expand Down