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

Clean up handling of connection file and info #863

Closed
wants to merge 3 commits into from

Conversation

blink1073
Copy link
Contributor

@blink1073 blink1073 commented Oct 24, 2022

KernelProvisioners are now explicitly responsible for the handling of the connection info and file, but the properties and methods of the ConnectionFileMixin continue to live on the KernelManager since they are needed when a provisioner is not available.

@blink1073 blink1073 marked this pull request as draft October 24, 2022 17:39
@blink1073
Copy link
Contributor Author

It turned out that trying to push ConnectionFileMixin down to the provisioner was too difficult. The transport and ip are inextricably linked to the socket creation methods, which require a context. Also do not have a provisioner until we first launch a kernel, so we cannot proxy properties from it until then.

@blink1073 blink1073 marked this pull request as ready for review October 31, 2022 01:15
@blink1073
Copy link
Contributor Author

ping @kevin-bates for another review

@kevin-bates
Copy link
Member

Hi @blink1073 - thanks for looking into this.

Given the caveats you ran into regarding transport and ip, and the fact that these changes will require any existing provisioner that does not derive from LocalProvisioner to change (so that the connection info it has obtained is made available to the KernelManager (and KernelClient) instances), I'm not sure if these changes are immediately warranted.

Is this PR serving as a small step forward to where we ultimately want to be? I suppose since this would be considered an API change, and we're nearing 8.0, this is the time to do this.

One thing we might be able to do to avoid breaking existing (non-LocalProvisioner) provisioners is to rework the post_launch() sequence to something like the following...

  1. Switch up the order of operations in _async_post_start_kernel() to call the provisioner's post_launch() method before the restarter.
  2. Add a call to self.parent.force_connection_info(self.connection_info) in KernelProvisionerBase.post_launch(). (Note: Existing provisioners may need to call super().post_launch() from their implementations if they haven't done so already.)
  3. Copy the contents of _force_connection_info() to the more public force_connection_info() and let _force_connection_info() forward to the more public method after logging a FutureWarning (i.e., deprecation warning).

There's still risk introduced by item 2, but the hope would be either those provisioners didn't implement post_launch() or they already call super().post_launch() - in which case this would still work despite the fact that they'd be returning connection information from launch_kernel().

What are your thoughts on the PR and the above? If you'd still like to move things forward, I'm okay with that, but I think, at a minimum, we need item 3 since "other" provisioners need to convey their connection info and they'd need to add the equivalent of item 2 somewhere.

Frankly, if 8.0 is the impetus here, I kinda like letting post_launch() be the place to convey the connection info.

@blink1073
Copy link
Contributor Author

@kevin-bates and I had a chat about this today. We agreed that it should be up to the provisioner whether it even needs to write a connection file, so we should keep the "forcing" logic on the manager to write the file for other consumers. However, we should also add a check in _force_connection_info to make sure that we are not changing the contents of an existing connection info file after launch. @kevin-bates is going to try out this idea with his remote provisioners, and we wiill make that change and backport it instead of making these API changes.

@blink1073 blink1073 closed this Nov 11, 2022
@blink1073 blink1073 deleted the update-provisioner branch November 11, 2022 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants