-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[App] Change app root / config path to be the app.py
parent directory
#15654
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ! However, this would break for some apps.
- 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)) | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 haveapp.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.
There was a problem hiding this comment.
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.
yes this is the expected user experience. this is especially important for transparency with enterprise customers. |
* chlog update * mypy: ignore mypy serve (#15631) (cherry picked from commit 38f2a91) * Enable Probot CheckGroup v3 (#15622) (cherry picked from commit bd870c6) * [App] Enable state broadcast with MultiNode (#15607) (cherry picked from commit 61c1f69) * [App] Resolve race condition to move ui files (#15398) (cherry picked from commit 2f0c039) * Make sure save_dir can be empty str (#15638) (cherry picked from commit c53dc20) * [App] Resolve bi-directional queue bug (#15642) (cherry picked from commit 0250c19) * Refactor checkgroup to avoid duplicated checks (#15633) Co-authored-by: Akihiro Nitta <nitta@akihironitta.com> Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com> (cherry picked from commit 1954764) * Delete unused TPU CI files (#15611) Co-authored-by: Akihiro Nitta <nitta@akihironitta.com> Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com> (cherry picked from commit a3edbec) * Update run_ptl_script.py (cherry picked from commit 4e8cf85) * [App] Accelerate Multi Node Startup Time (#15650) (cherry picked from commit 757413c) * [App] Change app root / config path to be the `app.py` parent directory (#15654) * Change app root / config path to be the `app.py` parent directory * Update CHANGELOG.md * mypy * Fix * Mypy (cherry picked from commit b3281eb) * Add LightningLite to top level imports (#15502) Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> (cherry picked from commit c32c435) * Upgrade GPU CI to PyTorch 1.13 (#15583) Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: Jirka <jirka.borovec@seznam.cz> (cherry picked from commit e87c11a) * Prevent artifactual "running from outside your current environment" error (#15647) Prevent warning when shutil.executable returns a symlink Co-authored-by: Luca Antiga <luca@lightning.ai> (cherry picked from commit 3248f33) * Fix ddp_spawn -> ddp fallback logic when on LSF cluster (#15657) Co-authored-by: awaelchli <aedu.waelchli@gmail.com> (cherry picked from commit cdb7006) * Include images with the mirror package (#15659) Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> (cherry picked from commit c06ea41) * [App] Rename failed -> error in tables (#15608) Co-authored-by: Raphael Randschau <nicolai86@users.noreply.github.com> Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> (cherry picked from commit ca83b50) * Improves the PanelFrontend docs (#14493) Co-authored-by: Marc Skov Madsen <masma@orsted.dk> Co-authored-by: thomas chaton <thomas@grid.ai> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> Co-authored-by: Felonious-Spellfire <felonious.spellfire@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Mansy <ahmed.mansy156@gmail.com> (cherry picked from commit 10a4b24) * add title and description to ServeGradio (#15639) * add title and description * update test * apply suggestions Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> (cherry picked from commit f9d906c) * Upgrade CI to PyTorch 1.13 (#15403) Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Akihiro Nitta <nitta@akihironitta.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> (cherry picked from commit 57ac548) * Fixed Import in Docs For Multinode Trainer Name Which does Not Exist (#15663) (cherry picked from commit 23f88cd) * Validate the combination of CloudCompute and BuildConfig (#14929) Co-authored-by: otaj <6065855+otaj@users.noreply.github.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Jirka <jirka.borovec@seznam.cz> (cherry picked from commit e5a865c) * add contributing guide to readme (cherry picked from commit 98af2bb) * Add Python 3.10 badge (#15681) (cherry picked from commit 8f44bb5) * fix(docs/app): broken links in the intermediate/web-ui section (#15691) (cherry picked from commit 4837df4) * Bump google-github-actions/setup-gcloud from 0 to 1 (#15671) Bumps [google-github-actions/setup-gcloud](https://github.com/google-github-actions/setup-gcloud) from 0 to 1. - [Release notes](https://github.com/google-github-actions/setup-gcloud/releases) - [Changelog](https://github.com/google-github-actions/setup-gcloud/blob/main/CHANGELOG.md) - [Commits](google-github-actions/setup-gcloud@v0...v1) --- updated-dependencies: - dependency-name: google-github-actions/setup-gcloud dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit c451288) * Update onnxruntime requirement from <1.13.0 to <1.14.0 in /requirements (#15672) Updates the requirements on [onnxruntime](https://github.com/microsoft/onnxruntime) to permit the latest version. - [Release notes](https://github.com/microsoft/onnxruntime/releases) - [Changelog](https://github.com/microsoft/onnxruntime/blob/main/docs/ReleaseManagement.md) - [Commits](microsoft/onnxruntime@v0.1.4...v1.13.1) --- updated-dependencies: - dependency-name: onnxruntime dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 7a8e2e9) * Bump google-github-actions/auth from 0 to 1 (#15675) Bumps [google-github-actions/auth](https://github.com/google-github-actions/auth) from 0 to 1. - [Release notes](https://github.com/google-github-actions/auth/releases) - [Changelog](https://github.com/google-github-actions/auth/blob/main/CHANGELOG.md) - [Commits](google-github-actions/auth@v0...v1) --- updated-dependencies: - dependency-name: google-github-actions/auth dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit e93c649) * Docs: Fix import for scikit in XGBoost template (#15693) (cherry picked from commit 9670fa8) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Enable Probot CheckGroup v4 (#15649) (cherry picked from commit 80e7538) * docs 5/n (#15669) * examples * fix few examples * Update pl_multinode.py * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 41f1a36) * fix(docs/app/lit_tabs): remove unused app_id, enable run instead (#15702) (cherry picked from commit befd3f6) * [App] Mock missing package imports when launching in the cloud (#15711) Co-authored-by: manskx <ahmed.mansy156@gmail.com> (cherry picked from commit f57160b) * Fix catimage import (#15712) (cherry picked from commit ee517f3) * Parse all lines in app file looking for shebangs to run commands. (#15714) fixed command parsing so that all lines in the file are parsed (cherry picked from commit 98bcb3d) * Bump coverage from 6.4.2 to 6.5.0 in /requirements (#15674) Bumps [coverage](https://github.com/nedbat/coveragepy) from 6.4.2 to 6.5.0. - [Release notes](https://github.com/nedbat/coveragepy/releases) - [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst) - [Commits](nedbat/coveragepy@6.4.2...6.5.0) --- updated-dependencies: - dependency-name: coverage dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 13eb2a1) * remove unused random_split import from tutorial (#15716) (cherry picked from commit b40ca0e) * Fix typo 'wether' (#15710) (cherry picked from commit 609b258) * releasing 1.8.2 Co-authored-by: Ethan Harris <ethanwharris@gmail.com> Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com> Co-authored-by: thomas chaton <thomas@grid.ai> Co-authored-by: Tianshu Wang <wang@tianshu.me> Co-authored-by: William Falcon <waf2107@columbia.edu> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> Co-authored-by: Luca Antiga <luca.antiga@gmail.com> Co-authored-by: Atharva Phatak <athp456@gmail.com> Co-authored-by: Luca Furst <rlfurst@gmail.com> Co-authored-by: Marc Skov Madsen <marc.skov.madsen@gmail.com> Co-authored-by: Aniket Maurya <theaniketmaurya@gmail.com> Co-authored-by: Rick Izzo <rlizzo@users.noreply.github.com> Co-authored-by: edenlightning <66261195+edenlightning@users.noreply.github.com> Co-authored-by: 罗崚骁(LUO Lingxiao) <function2-llx@outlook.com> Co-authored-by: Yurij Mikhalevich <yurij@grid.ai> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Nash <nash@lightning.ai> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: yiftachbeer <yiftach.beer@gmail.com> Co-authored-by: dymil <30931139+dymil@users.noreply.github.com>
What does this PR do?
The logic around finding the root is complex and has some failure modes (e.g. having a .lightning file in a parent directory for any reason will cause that whole directory to get uploaded).
This PR makes it so that the root directory === the parent of the entrypoint file
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃