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

Jump to bottom

[App] Add --zip option to cp command #16798

Merged
merged 21 commits into from
Mar 7, 2023
Merged

[App] Add --zip option to cp command #16798

merged 21 commits into from
Mar 7, 2023

Conversation

luca3rd
Copy link
Contributor

@luca3rd luca3rd commented Feb 17, 2023 •
edited
Loading

What does this PR do?

  • Adds command to download content from the Lightning Filesystem as a zip file.
➜ lightning pwd
/default-project

➜ lightning ls
wide-tan-ryq8

➜ lightning cp r:wide-tan-ryq8 . --zip
Downloaded to ./wide-tan-ryq8.zip

➜ unzip -l ./wide-tan-ryq8.zip 
Archive:  ./wide-tan-ryq8.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  02-17-2023 11:34   code/.lightning-app-sync
       41  02-17-2023 11:34   code/app.py
---------                     -------
       41                     2 files
  • Fixes bug in cp._sanitize_path
  • Refactors token client
  • Bumps rich version to support progress bars w/ indefinite size
Before submitting
  • Was this discussed/agreed via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

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:

Reviewer checklist
  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

cc @Borda

@github-actions github-actions bot added the app (removed) Generic label for Lightning App package label Feb 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2023 •
edited
Loading

⚡ Required checks status: All passing 🟢

Groups summary

🟢 pytorch_lightning: Tests workflow
Check ID Status
pl-cpu (macOS-11, lightning, 3.8, 1.11) success ✅
pl-cpu (macOS-11, lightning, 3.9, 1.12) success ✅
pl-cpu (macOS-11, lightning, 3.10, 1.13) success ✅
pl-cpu (macOS-11, lightning, 3.8, 1.11, oldest) success ✅
pl-cpu (macOS-11, lightning, 3.9, 2.0, pre) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.9, 1.11) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.10, 1.12) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.10, 1.13) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.8, 1.11, oldest) success ✅
pl-cpu (ubuntu-20.04, lightning, 3.9, 2.0, pre) success ✅
pl-cpu (windows-2022, lightning, 3.9, 1.11) success ✅
pl-cpu (windows-2022, lightning, 3.10, 1.12) success ✅
pl-cpu (windows-2022, lightning, 3.10, 1.13) success ✅
pl-cpu (windows-2022, lightning, 3.8, 1.11, oldest) success ✅
pl-cpu (windows-2022, lightning, 3.9, 2.0, pre) success ✅
pl-cpu (macOS-11, pytorch, 3.8, 1.13) success ✅
pl-cpu (ubuntu-20.04, pytorch, 3.8, 1.13) success ✅
pl-cpu (windows-2022, pytorch, 3.8, 1.13) success ✅

These checks are required after the changes to requirements/pytorch/extra.txt.

🟢 pytorch_lightning: Azure GPU
Check ID Status
pytorch-lightning (GPUs) success ✅

These checks are required after the changes to requirements/pytorch/extra.txt.

🟢 pytorch_lightning: Benchmarks
Check ID Status
pytorch-lightning.Benchmark success ✅

These checks are required after the changes to requirements/pytorch/extra.txt.

🟢 pytorch_lightning: Azure HPU
Check ID Status
pytorch-lightning (HPUs) success ✅

These checks are required after the changes to requirements/pytorch/extra.txt.

🟢 pytorch_lightning: Docs
Check ID Status
make-doctest (pytorch) success ✅
make-html (pytorch) success ✅

These checks are required after the changes to requirements/pytorch/extra.txt.

🟢 pytorch_lightning: Docker
Check ID Status
build-cuda (3.9, 1.11, 11.3.1) success ✅
build-cuda (3.9, 1.12, 11.6.1) success ✅
build-cuda (3.9, 1.13, 11.7.1) success ✅
build-hpu (1.5.0, 1.11.0) success ✅
build-ipu (3.9, 1.13) success ✅
build-NGC success ✅
build-pl (3.9, 1.11, 11.3.1) success ✅
build-pl (3.9, 1.12, 11.6.1) success ✅
build-pl (3.9, 1.13, 11.7.1) success ✅
build-xla (3.8, 1.12) success ✅

These checks are required after the changes to requirements/pytorch/extra.txt.

🟢 lightning_app: Tests workflow
Check ID Status
app-pytest (macOS-11, lightning, 3.8, latest) success ✅
app-pytest (macOS-11, lightning, 3.8, oldest) success ✅
app-pytest (macOS-11, app, 3.9, latest) success ✅
app-pytest (ubuntu-20.04, lightning, 3.8, latest) success ✅
app-pytest (ubuntu-20.04, lightning, 3.8, oldest) success ✅
app-pytest (ubuntu-20.04, app, 3.9, latest) success ✅
app-pytest (windows-2022, lightning, 3.8, latest) success ✅
app-pytest (windows-2022, lightning, 3.8, oldest) success ✅
app-pytest (windows-2022, app, 3.8, latest) success ✅

These checks are required after the changes to src/lightning/app/cli/commands/cp.py, src/lightning/app/cli/commands/ls.py, src/lightning/app/utilities/auth.py, src/lightning/app/utilities/logs_socket_api.py, tests/tests_app/cli/test_cp.py, requirements/app/base.txt.

🟢 lightning_app: Examples
Check ID Status
app-examples (macOS-11, lightning, 3.9, latest) success ✅
app-examples (macOS-11, lightning, 3.9, oldest) success ✅
app-examples (macOS-11, app, 3.9, latest) success ✅
app-examples (ubuntu-20.04, lightning, 3.9, latest) success ✅
app-examples (ubuntu-20.04, lightning, 3.9, oldest) success ✅
app-examples (ubuntu-20.04, app, 3.9, latest) success ✅
app-examples (windows-2022, lightning, 3.9, latest) success ✅
app-examples (windows-2022, lightning, 3.9, oldest) success ✅
app-examples (windows-2022, app, 3.9, latest) success ✅

These checks are required after the changes to src/lightning/app/cli/commands/cp.py, src/lightning/app/cli/commands/ls.py, src/lightning/app/utilities/auth.py, src/lightning/app/utilities/logs_socket_api.py, requirements/app/base.txt.

🟢 lightning_app: Azure
Check ID Status
App.cloud-e2e success ✅

These checks are required after the changes to src/lightning/app/cli/commands/cp.py, src/lightning/app/cli/commands/ls.py, src/lightning/app/utilities/auth.py, src/lightning/app/utilities/logs_socket_api.py, requirements/app/base.txt.

🟢 lightning_app: Docs
Check ID Status
make-doctest (app) success ✅
make-html (app) success ✅

These checks are required after the changes to src/lightning/app/cli/commands/cp.py, src/lightning/app/cli/commands/ls.py, src/lightning/app/utilities/auth.py, src/lightning/app/utilities/logs_socket_api.py, requirements/app/base.txt.

🟢 mypy
Check ID Status
mypy success ✅

These checks are required after the changes to requirements/app/base.txt, requirements/pytorch/extra.txt, src/lightning/app/cli/commands/cp.py, src/lightning/app/cli/commands/ls.py, src/lightning/app/utilities/auth.py, src/lightning/app/utilities/logs_socket_api.py.

🟢 install
Check ID Status
install-pkg (ubuntu-22.04, app, 3.8) success ✅
install-pkg (ubuntu-22.04, app, 3.10) success ✅
install-pkg (ubuntu-22.04, fabric, 3.8) success ✅
install-pkg (ubuntu-22.04, fabric, 3.10) success ✅
install-pkg (ubuntu-22.04, pytorch, 3.8) success ✅
install-pkg (ubuntu-22.04, pytorch, 3.10) success ✅
install-pkg (ubuntu-22.04, lightning, 3.8) success ✅
install-pkg (ubuntu-22.04, lightning, 3.10) success ✅
install-pkg (ubuntu-22.04, notset, 3.8) success ✅
install-pkg (ubuntu-22.04, notset, 3.10) success ✅
install-pkg (macOS-12, app, 3.8) success ✅
install-pkg (macOS-12, app, 3.10) success ✅
install-pkg (macOS-12, fabric, 3.8) success ✅
install-pkg (macOS-12, fabric, 3.10) success ✅
install-pkg (macOS-12, pytorch, 3.8) success ✅
install-pkg (macOS-12, pytorch, 3.10) success ✅
install-pkg (macOS-12, lightning, 3.8) success ✅
install-pkg (macOS-12, lightning, 3.10) success ✅
install-pkg (macOS-12, notset, 3.8) success ✅
install-pkg (macOS-12, notset, 3.10) success ✅
install-pkg (windows-2022, app, 3.8) success ✅
install-pkg (windows-2022, app, 3.10) success ✅
install-pkg (windows-2022, fabric, 3.8) success ✅
install-pkg (windows-2022, fabric, 3.10) success ✅
install-pkg (windows-2022, pytorch, 3.8) success ✅
install-pkg (windows-2022, pytorch, 3.10) success ✅
install-pkg (windows-2022, lightning, 3.8) success ✅
install-pkg (windows-2022, lightning, 3.10) success ✅
install-pkg (windows-2022, notset, 3.8) success ✅
install-pkg (windows-2022, notset, 3.10) success ✅

These checks are required after the changes to src/lightning/app/cli/commands/cp.py, src/lightning/app/cli/commands/ls.py, src/lightning/app/utilities/auth.py, src/lightning/app/utilities/logs_socket_api.py, requirements/app/base.txt, requirements/pytorch/extra.txt.

🟢 link-check
Check ID Status
markdown-link-check success ✅

These checks are required after the changes to src/lightning/app/CHANGELOG.md, src/lightning/pytorch/CHANGELOG.md.

Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

@luca3rd luca3rd force-pushed the luca3rd/download-artifacts branch from 8c63ac3 to d95c3f7 Compare February 17, 2023 17:05
@luca3rd luca3rd self-assigned this Feb 17, 2023
@luca3rd luca3rd added this to the v1.9.x milestone Feb 17, 2023
@tchaton
Copy link
Contributor

tchaton commented Feb 17, 2023 •
edited
Loading

@luca3rd This is kinda of a confusing API.

Won't it be better to have lightning cp r:. . --zip, so we don't introduce a new command ?

Best,
T.C

@luca3rd luca3rd changed the title [App] Add lightning zip command [App] Add --zip option to cp command Feb 17, 2023
tchaton
tchaton approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Nice work !

src/lightning/app/cli/commands/cp.py Outdated Show resolved Hide resolved
src/lightning/app/cli/commands/cp.py Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Feb 17, 2023
@luca3rd luca3rd enabled auto-merge (squash) February 18, 2023 05:34
@luca3rd luca3rd requested a review from Borda as a code owner February 20, 2023 03:54
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Feb 20, 2023
@luca3rd luca3rd disabled auto-merge February 20, 2023 04:09
@codecov
Copy link

codecov bot commented Feb 20, 2023 •
edited
Loading

Codecov Report

Merging #16798 (4117266) into master (54147e0) will increase coverage by 37%.
The diff coverage is 98%.

❗ Current head 4117266 differs from pull request most recent head 1f579b1. Consider uploading reports for the commit 1f579b1 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #16798      +/-   ##
==========================================
+ Coverage      42%      79%     +37%     
==========================================
  Files         434      414      -20     
  Lines       31588    31340     -248     
==========================================
+ Hits        13304    24655   +11351     
+ Misses      18284     6685   -11599

Borda
Borda reviewed Feb 20, 2023
View reviewed changes
requirements/app/base.txt Outdated Show resolved Hide resolved
requirements/pytorch/extra.txt Show resolved Hide resolved
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Feb 27, 2023
@luca3rd luca3rd force-pushed the luca3rd/download-artifacts branch from f135a6d to f05809e Compare February 27, 2023 15:50
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts labels Feb 27, 2023
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Mar 2, 2023
luca3rd and others added 21 commits March 6, 2023 10:23
@luca3rd
Add lightning zip command
e492761
@luca3rd
Fix papercuts
e75ea1d
@pre-commit-ci @luca3rd
[pre-commit.ci] auto fixes from pre-commit.com hooks
71ddd59 
for more information, see https://pre-commit.ci
@luca3rd
Fix long line
4496540
@luca3rd
Fix mypy
028aaef
@luca3rd
Migrate zip to cp --zip
c4f7544
@luca3rd
fixup! Migrate zip to cp --zip
8d3b38e
@pre-commit-ci @luca3rd
[pre-commit.ci] auto fixes from pre-commit.com hooks
d19711d 
for more information, see https://pre-commit.ci
@luca3rd
PR feedback
f0f3f5e
@luca3rd
Skip sanitize path test on windows
078ebfe
@luca3rd
Don't make live calls in unit test
3221471
@pre-commit-ci @luca3rd
[pre-commit.ci] auto fixes from pre-commit.com hooks
ab47476 
for more information, see https://pre-commit.ci
@luca3rd
Remove mistake from test
5ff62d5
@luca3rd
Fix test_cp_local_to_remote
64851e2
@luca3rd
Mock LightningClient before calling cp
473b166
@luca3rd
Mock rich.live
e87f5a8
@luca3rd
fixup! Mock rich.live
3e7a571
@luca3rd
Try bump rich version
978e848
@luca3rd
fixup! Try bump rich version
46457dc
@luca3rd @Borda
Update requirements/app/base.txt
2e6f4a5 
Co-authored-by: Jirka Borovec <[email protected]>
@luca3rd
Update changelogs
1f579b1
@luca3rd luca3rd force-pushed the luca3rd/download-artifacts branch from 4117266 to 1f579b1 Compare March 6, 2023 15:24
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Mar 6, 2023
@Borda Borda merged commit 77e1cc9 into master Mar 7, 2023
@Borda Borda deleted the luca3rd/download-artifacts branch March 7, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda left review comments

@justusschock justusschock justusschock approved these changes

@tchaton tchaton tchaton approved these changes

@nicolai86 nicolai86 nicolai86 approved these changes

@lantiga lantiga Awaiting requested review from lantiga lantiga is a code owner

@awaelchli awaelchli Awaiting requested review from awaelchli

@hhsecond hhsecond Awaiting requested review from hhsecond

@ethanwharris ethanwharris Awaiting requested review from ethanwharris ethanwharris is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@carmocca carmocca Awaiting requested review from carmocca

Assignees

@luca3rd luca3rd

Labels
app (removed) Generic label for Lightning App package pl Generic label for PyTorch Lightning package ready PRs ready to be merged
Projects
None yet
Milestone
v1.9.x
Development

Successfully merging this pull request may close these issues.
6 participants
@luca3rd @tchaton @nicolai86 @awaelchli @Borda @justusschock