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

Only cache ports if the cache_ports flag is set to True #492

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Only cache ports if the cache_ports flag is set to True #492

merged 1 commit into from
Oct 17, 2019

Conversation

martinRenou
Copy link
Member

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

@martinRenou - thank you for adding the flag. Since MultiKernelManager can manage different types (and localities) of kernels, the decision to cache ports for a given KernelManager should, ideally, come from the KernelManager instance.

jupyter_client/multikernelmanager.py Outdated Show resolved Hide resolved
jupyter_client/multikernelmanager.py Outdated Show resolved Hide resolved
@SylvainCorlay
Copy link
Member

@martinRenou - thank you for adding the flag. Since MultiKernelManager can manage different types (and localities) of kernels, the decision to cache ports for a given KernelManager should, ideally, come from the KernelManager instance.

This sounds like a convoluted design while we are not quite sure why disabling that behavior is required in the first place - and you can still disable it completely.

Would you like to have a quick chat to discuss this viva voce?

@kevin-bates
Copy link
Member

kevin-bates commented Oct 15, 2019

@SylvainCorlay Sure - I think that would be great. We can meet here: https://ibm.webex.com/meet/kevin.bates unless you have something else.

@SylvainCorlay
Copy link
Member

I am on the room.

@martinRenou
Copy link
Member Author

As discussed during the meeting. We'll add the flag in KernelManager, in case the MultiKernelManager manages remote and local kernels (this caching logic only makes sense in the case of a local kernels).

@martinRenou martinRenou changed the title Only cache ports if the cache_ports flag is set to True WIP - Only cache ports if the cache_ports flag is set to True Oct 15, 2019
@martinRenou martinRenou changed the title WIP - Only cache ports if the cache_ports flag is set to True Only cache ports if the cache_ports flag is set to True Oct 16, 2019
@martinRenou
Copy link
Member Author

Would you like to give it a look @kevin-bates ?

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Looks good @martinRenou - thank you!

@SylvainCorlay
Copy link
Member

Go!

@SylvainCorlay SylvainCorlay merged commit fb91900 into jupyter:master Oct 17, 2019
@martinRenou martinRenou deleted the cache_ports branch October 17, 2019 12:56
@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyter-client-6-0-0/3414/1

@kevin-bates
Copy link
Member

@martinRenou - I took JC 6.0 for a spin in my EG environment yesterday and had an issue with cache_ports during shutdown because the cached ports weren't in currently_used_ports resulting in a KeyError.

Clearly, the remote kernels don't need (or want) to cache the ports, and I need to change cache_ports to false for these kinds of manager instances. However, I figured I'd be able to do this via the command line (e.g., --KernelManager.cache_ports = False) but now realize that the cache_ports traitlet isn't configurable. Moreover, for subclasses to set cache_ports = False, they must implement a constructor (or whatever __init__() is called in python) because the cached ports are captured immediately following the manager's instantiation. Once I created the constructor, I was able to manage the remote kernels w/o a KeyError.

My question is, was it intentional to not have the cache_ports traitlet configurable (I.e., config=True)? I'm curious how someone would set it to false if they still wanted tcp without having to modify their code, create a new release, etc.? I think being able to do something like --XXXKernelManager=False while still allowing KernelManager instances to cache ports would be nice.

I may not fully understand how traitlets work, so wanted to get your take on how users should change cache_ports.

cc: @SylvainCorlay

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