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

Fix %run_viz using old process in jupyter notebook #2267

Merged
merged 6 commits into from
Feb 10, 2025

Conversation

ravi-kumar-pilla
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla commented Feb 3, 2025

Description

Resolves #1823

NOTE: I increased the max attempts of ports to 10 but something seems off with the entire process. So the question is should we treat each jupyter cell a separate viz instance when someone does %run_viz ? With the current approach, the issue of using old running process is fixed but the users can run %run_viz only 10 times in a notebook session

image

Development notes

  • Add _find_available_port logic placed in run command to %run_viz
  • Remove allocate_port method

QA notes

  • All tests should pass
  • You can test %run_viz on notebook and it should use different ports for different cell runs.

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

Signed-off-by: ravi_kumar_pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi_kumar_pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla marked this pull request as ready for review February 3, 2025 21:14
@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Feb 10, 2025

hi Ravi, i am debating whether we should spend more time on this. %run_viz is not used much. It was broken for 6-7 months and nobody complained. Also, given with the new way to 'Visualise pipelines in notebooks', this might become old Let's increase it to max 10 and see if we get users complaining -- then we can decide what to do. wdyt @astrojuanlu ?

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Agreed with @rashidakanchwala , in line with what I said in #1823 (comment), probably we will be de-emphasizing this over time. Still, if there's no more work needed here and given that this actually removed code 😄 let's merge!

Signed-off-by: ravi_kumar_pilla <ravi_kumar_pilla@mckinsey.com>
Signed-off-by: ravi_kumar_pilla <ravi_kumar_pilla@mckinsey.com>
@ravi-kumar-pilla ravi-kumar-pilla merged commit 0690aaf into main Feb 10, 2025
37 checks passed
@ravi-kumar-pilla ravi-kumar-pilla deleted the fix/run-viz-jupyter branch February 10, 2025 15:38
@Huongg Huongg mentioned this pull request Mar 6, 2025
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.

%run_viz Jupyter line magic inconsistency
3 participants