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

Add port in use check to prevent browser redirecting incorrectly for kedro viz #2176

Merged
merged 9 commits into from
Nov 8, 2024

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Nov 6, 2024

Description

Related to: #2052

If there is an existing Kedro Viz server running, kedro viz run will redirect the browser to that one when a second instance of kedro viz is launched using the same port and the error message from uvicorn is missed.

Development notes

Check that the port is in use before running the server and increment it until a max 5 times to find an alternative free port, if none are found send error message to users no free ports available and suggest them to use --port.

QA notes

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@rashidakanchwala
Copy link
Contributor

hey @SajidAlamQB , can we not increment the port number here, directly, if 4141 is occupied, go to 4142, if that is occupied which is it always, go to 4143?

@rashidakanchwala
Copy link
Contributor

maybe we put a specific range from 4141, to 4145, and if all the ports in this range are busy, then they tell us which one through --port.

@SajidAlamQB SajidAlamQB changed the title Add port in use check to prevent for kedro viz Add port in use check to prevent browser redirecting incorrectly for kedro viz Nov 6, 2024
@SajidAlamQB
Copy link
Contributor Author

maybe we put a specific range from 4141, to 4145, and if all the ports in this range are busy, then they tell us which one through --port.

This is a good middle approach we definitely can do.

@rashidakanchwala
Copy link
Contributor

@SajidAlamQB , thanks for this , will review this once tests are passing, can we also add unit-testing to this, if we can mock ports being occupied ?

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

Works well and LGTM. Thank you @SajidAlamQB 💯

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @SajidAlamQB

@SajidAlamQB SajidAlamQB merged commit 11f1608 into main Nov 8, 2024
40 checks passed
@SajidAlamQB SajidAlamQB deleted the bugfix/viz-using-same-service branch November 8, 2024 12:43
@jitu5 jitu5 mentioned this pull request Nov 20, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants