[runtime env] Add plugin name to internal URI format and add GC for py_modules #20009
Conversation
|
This is ready for review, |
There was a problem hiding this comment.
Looks pretty good, but I would prefer if we can keep this protocol | URI thing purely an internal implementation detail and minimally scoped to avoid issues like the job submission needing to worry about it.
Based on my understanding, we only need this in order to tell the agent which plugin it should call delete on when GC happens, right? If that's the case, what if we just do the following:
- Prepend
plugin_name|to the URIs when we callget_uristo pass it into C++-land. - In the agent, when
DeleteURIis called, parse out the plugin_name and URI before calling into a specific plugin for deletion.
This should isolate this to just those two specific places instead of needing to inject this logic all over the place. What do you think?
|
Nice, that's better! I'll update this PR. |
architkulkarni
added
the
@author-action-required
There was a problem hiding this comment.
@architkulkarni this is looking way better. I'm a little disconcerted that we have to do the string parsing in C++, but it may be ok.
Is it possible that we just make this a more structured format in the protobuf instead of just a list of strings? We could have the protobuf be:
message InternalURI {
string protocol = 1;
string uri = 2;
}
Then the GCS could just look at protocol and we could avoid the whole parsing thing. Would that work? Sorry for not thinking of it sooner, it might be a cleaner way to do this... if this is a big change I'm ok with the status quo
| if (unused_uris_.count(uri)) { | ||
| unused_uris_.erase(uri); | ||
| } |
There was a problem hiding this comment.
what's the context for this change?
There was a problem hiding this comment.
As far as I can tell it wasn't doing anything, it was just getting populated and deleted but was never read anywhere else. It might have been leftover from some earlier implementation
| runtime_env_manager_ = std::make_unique<RuntimeEnvManager>( | ||
| /*deleter=*/[this](const std::string &plugin_uri, auto cb) { | ||
| // A valid runtime env URI is of the form "plugin|protocol://hash". | ||
| std::string plugin_sep = "|"; | ||
| std::string protocol_sep = "://"; | ||
| auto plugin_end_pos = plugin_uri.find(plugin_sep); | ||
| auto protocol_end_pos = plugin_uri.find(protocol_sep); | ||
| if (protocol_end_pos == std::string::npos || | ||
| plugin_end_pos == std::string::npos) { | ||
| RAY_LOG(ERROR) << "Plugin URI must be of form " | ||
| << "<plugin>|<protocol>://<hash>, got " << plugin_uri; |
There was a problem hiding this comment.
Oh, ouch, I didn't realize we'd need to parse this in C++
@edoakes no worries, I also should have thought of this. This sounds good, you mean It's a conceptually simple change but I think it'll require a lot of updates across the Cython and C++, so how about we do it in a followup PR? |
|
@architkulkarni I'm fine w/ that |
|
and yes meant plugin not protocol |
Why are these changes needed?
plugin|protocol://pkg_name. We need the plugin name in the URI because the runtime env agent needs to know which plugin to call thedeletemethod of just based on the URI.py_modulesRelated issue number
Closes #19775
Checks
scripts/format.shto lint the changes in this PR.