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] Auto-upgrade / detect environment mis-match from the CLI #15434

Merged
merged 8 commits into from
Nov 2, 2022
Merged

[App] Auto-upgrade / detect environment mis-match from the CLI #15434

merged 8 commits into from
Nov 2, 2022

Conversation

ethanwharris
Copy link
Member

@ethanwharris ethanwharris commented Oct 31, 2022 •
edited
Loading

What does this PR do?

Adds utilities to the lightning app CLI to avoid the need for python -m XXX:

  • auto-upgrade (after prompt) if a newer version is available
  • auto redirect to the current environment (with prompted install if needed) when running lightning out of a different Python from the current one on the path

Note: The test here is quite minimal for now as this can't be tested without mocking out all of the important pieces (at which point the test wouldn't catch anything)

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

  • Was this discussed/approved 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:

  • 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

Did you have fun?

Make sure you had fun coding 🙃

@ethanwharris ethanwharris added app (removed) Generic label for Lightning App package app:cli labels Oct 31, 2022
@ethanwharris ethanwharris added this to the v1.8 milestone Oct 31, 2022
tchaton
tchaton reviewed Oct 31, 2022
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.

Looks neat so far.

src/lightning_app/utilities/cli_helpers.py Show resolved Hide resolved
@Borda Borda modified the milestones: v1.8, v1.9 Oct 31, 2022
@ethanwharris ethanwharris marked this pull request as ready for review November 1, 2022 12:19
tchaton
tchaton approved these changes Nov 2, 2022
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.

LGTM !

@williamFalcon williamFalcon merged commit 555257a into master Nov 2, 2022
@williamFalcon williamFalcon deleted the feature/cli_auto_upgrade branch November 2, 2022 21:13
Borda
Borda reviewed Nov 2, 2022
View reviewed changes
src/lightning_app/__init__.py
@@ -37,6 +37,8 @@
if module_available("lightning_app.components.demo"):
from lightning_app.components import demo # noqa: F401

__package_name__ = "lightning_app".split(".")[0]
Copy link
Member

Choose a reason for hiding this comment

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

?

Borda
Borda reviewed Nov 2, 2022
View reviewed changes
tests/tests_app/utilities/test_cli_helpers.py


@pytest.mark.parametrize(
"releases, current_version, newer_version",
Copy link
Member

Choose a reason for hiding this comment

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

how about dev version if it is installed from the source?

Borda pushed a commit that referenced this pull request Nov 5, 2022
@ethanwharris @Borda
[App] Auto-upgrade / detect environment mis-match from the CLI (#15434)
1b525e7 
* Add auto-upgrade from the CLI and check for current env

* No longer require `python -m` in docs

* Tabs -> spaces

* Ignore pre-releases

* Test + docs

(cherry picked from commit 555257a)
@otaj otaj mentioned this pull request Nov 7, 2022
Closed
@ethanwharris ethanwharris modified the milestones: v1.9, v1.8.x Nov 10, 2022
lexierule pushed a commit that referenced this pull request Nov 10, 2022
@ethanwharris @lexierule
[App] Auto-upgrade / detect environment mis-match from the CLI (#15434)
0299d4e 
* Add auto-upgrade from the CLI and check for current env

* No longer require `python -m` in docs

* Tabs -> spaces

* Ignore pre-releases

* Test + docs

(cherry picked from commit 555257a)
@nicolai86 nicolai86 mentioned this pull request Dec 7, 2022
Merged
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

@tchaton tchaton tchaton approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli

@manskx manskx Awaiting requested review from manskx

@hhsecond hhsecond Awaiting requested review from hhsecond

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

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
app (removed) Generic label for Lightning App package
Projects
None yet
Milestone
v1.8.x
Development

Successfully merging this pull request may close these issues.
4 participants
@ethanwharris @Borda @tchaton @williamFalcon