-
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
Selects random port for lightning app when running multiple apps locally #15819
Conversation
⚡ Required checks status: All passing 🟢Groups summary🟢 lightning_app: Tests workflow
These checks are required after the changes to 🟢 lightning_app: Examples
These checks are required after the changes to 🟢 lightning_app: Azure
These checks are required after the changes to 🟢 lightning_app: Docs
These checks are required after the changes to 🟢 mypy
These checks are required after the changes to 🟢 installThese checks are required after the changes to Thank you for your contribution! 💜
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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 !
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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!
port = _find_lit_app_port(constants.APP_SERVER_PORT) | ||
constants.APP_SERVER_PORT = port |
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.
If there is really no other way than making it a global variable, then let's move it out of the "constants" file? Clearly, if it changes at runtime, it's no longer a constant.
Perhaps constants.py
could contain APP_SERVER_DEFAULT_PORT=7501
and then, I don't know, maybe lightning_app.py
could hold the global var for the port that's being used.
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.
Thanks for the review @awaelchli I will update per your suggestion
What does this PR do?
Selects a random port when running a lightning app to avoid port collisions when running multiple apps locally.
Does your PR introduce any breaking changes? If yes, please list them.
No breaking changes. Most of the tests expect port 7501 as the hardcoded port for locally running lightning apps. This PR still uses 7501 as the default port for lightning apps and will only use a different port when this port is taken.
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 🙃
cc @Borda