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

Do not modify PACKAGE_NAME on install #15493

Merged
merged 36 commits into from
Nov 4, 2022
Merged

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Nov 3, 2022

What does this PR do?

See title.

lightning is no longer installed when PACKAGE_NAME=lite|app|pytorch is installed.

Part of #15474

Fixes #15489 by reverting most of #14517

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

None

cc @carmocca @akihironitta @Borda

@carmocca carmocca added the ci Continuous Integration label Nov 3, 2022
@carmocca carmocca self-assigned this Nov 3, 2022
@carmocca carmocca force-pushed the ci/avoid-writing-pkg-name branch from 75cd063 to 3531b0d Compare November 3, 2022 02:02
@Borda
Copy link
Member

Borda commented Nov 3, 2022

The tricky part is that the package selection needs to be included in the build package, so we can instead of rewrite setup.py and write it to an extra file, but it still need to be dumped somewhere and distributed with sdist/wheel

@carmocca carmocca force-pushed the ci/avoid-writing-pkg-name branch from ffba74d to e03646f Compare November 3, 2022 03:25
@carmocca carmocca force-pushed the ci/avoid-writing-pkg-name branch 2 times, most recently from c2903e5 to fdff3a8 Compare November 3, 2022 03:56
@carmocca carmocca force-pushed the ci/avoid-writing-pkg-name branch from fdff3a8 to 52c2b5a Compare November 3, 2022 03:58
@carmocca carmocca requested a review from manskx as a code owner November 3, 2022 04:20
@mergify mergify bot removed the ready PRs ready to be merged label Nov 4, 2022
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Nov 4, 2022
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 !

@carmocca
Copy link
Contributor Author

carmocca commented Nov 4, 2022

There's been a small change to the checkgroup config: f81ae59 so this will need force-merge. CI was green the commit before: https://github.com/Lightning-AI/lightning/actions/runs/3393882442/jobs/5643540143

@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Nov 4, 2022
.github/workflows/ci-app-tests.yml Show resolved Hide resolved
requirements/pytorch/base.txt Show resolved Hide resolved
src/lightning_app/testing/testing.py Show resolved Hide resolved
tests/tests_app/cli/test_cli.py Show resolved Hide resolved
@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Nov 4, 2022
@lantiga lantiga merged commit f392180 into master Nov 4, 2022
@lantiga lantiga deleted the ci/avoid-writing-pkg-name branch November 4, 2022 16:51
Borda pushed a commit that referenced this pull request Nov 5, 2022
* Do not modify PACKAGE_NAME on install

* Fix ci pkg action

* Required

* Typos

* Apply suggestions from code review

* Undo defaults

* Cleanup

* Implement idea

* Fuck

* Apps mock fix

* Fix app-pytest with PKG_NAME=app

* Justus suggestion

* Debug Windows

* Update setup.py

Co-authored-by: Adrian Wälchli <[email protected]>

* Revert "Debug Windows"

This reverts commit 9fe3ba3.

* SSH action

* Crazy bug

* Revert "SSH action"

This reverts commit 5061e8e.

* Package import step

* Avoid env conflict

* Debug

* Whitespace

* Try removing existing lite build

* This should be redundant now

* Add back env now that source-lit is gone

* Remove download artifact

* checkgroup

* TODOs suggested by Jirka

* _

* Revert "_". These are local variables, do not need protected

This reverts commit 8340b85.

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>
(cherry picked from commit f392180)
lexierule pushed a commit that referenced this pull request Nov 10, 2022
* Do not modify PACKAGE_NAME on install

* Fix ci pkg action

* Required

* Typos

* Apply suggestions from code review

* Undo defaults

* Cleanup

* Implement idea

* Fuck

* Apps mock fix

* Fix app-pytest with PKG_NAME=app

* Justus suggestion

* Debug Windows

* Update setup.py

Co-authored-by: Adrian Wälchli <[email protected]>

* Revert "Debug Windows"

This reverts commit 9fe3ba3.

* SSH action

* Crazy bug

* Revert "SSH action"

This reverts commit 5061e8e.

* Package import step

* Avoid env conflict

* Debug

* Whitespace

* Try removing existing lite build

* This should be redundant now

* Add back env now that source-lit is gone

* Remove download artifact

* checkgroup

* TODOs suggested by Jirka

* _

* Revert "_". These are local variables, do not need protected

This reverts commit 8340b85.

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>
(cherry picked from commit f392180)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration ready PRs ready to be merged
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Distribute Lite's code with PyTorch Lightning
7 participants