Closed blink1073 closed 1 year ago
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.
ping @kevin-bates for another review
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...
_async_post_start_kernel()
to call the provisioner's post_launch()
method before the restarter.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.)_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.
@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.
KernelProvisioners
are now explicitly responsible for the handling of the connection info and file, but the properties and methods of theConnectionFileMixin
continue to live on theKernelManager
since they are needed when a provisioner is not available.