-
Notifications
You must be signed in to change notification settings - Fork 18
feat: refactor runtime system for extensible multi-entrypoint support #750
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
Conversation
888b9b7 to
d71f827
Compare
| """Get a list of all available runtimes.""" | ||
| return [] | ||
|
|
||
| def get_runtime(self, entrypoint: str) -> Optional[T]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need not exist. <*>Runtime.from_context is sufficient here as context contains entrypoint.
| return None | ||
| return [] | ||
|
|
||
| return [os.path.join(directory, f) for f in python_files] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! :)
However, I do think we need to filter this. The ScriptExecutor does this:
for func_name in ["main", "run", "execute"]:
if hasattr(module, func_name):
so let's ensure that the files have these defined, yea?
| """ | ||
| working_dir = self.context.runtime_dir or os.getcwd() | ||
| script_path = get_user_script(working_dir, entrypoint=self.context.entrypoint) | ||
| working_dir = os.getcwd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend keeping this:
working_dir = self.context.runtime_dir or os.getcwd()
Same below.
|
|
||
| return runtimes | ||
|
|
||
| def get_runtime(self, entrypoint: str) -> Optional[UiPathScriptRuntime]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above this method is redundant -- can be safely moved to new_runtime method
| try: | ||
| runtime_factory = generate_runtime_factory() | ||
| # Last one is the Python script runtime factory | ||
| runtime_factory = Middlewares._runtime_factories[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not have the fallback logic. Ideally we want to have
Middlewares.get_runtime_factory(entrypoint)
here and in cli_run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the LangGraph eval and run logic are still handled in their own middleware - we will refactor those in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is, if someone imports langgraph library but doesn't define a graph,Middlewares._runtime_factories[-1] will end up using the langgraph factory and will never fallback to the script runtimefactory.
| return [] | ||
|
|
||
| @classmethod | ||
| def get_runtime(cls, entrypoint: str) -> Optional[UiPathBaseRuntime]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this to get_runtime_factory(entrypoint)?
Maybe in order to support this, we need to update register_runtime_factory to discover all entrypoints and maintain a map between entrypoint->RuntimeFactory.
This would mean discovering everytime the agent runs but looks like we don't have a choice right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, nvm - usually there's only one valid runtime factory, so let's change impl to
@classmethod
def get_runtime_factory(cls, entrypoint: str) -> UiPathRuntimeFactory:
for factory in..:
try:
factory.new_runtime(entrypoint=entrypoint)
return factory
except:
log('entrypoint cannot be instantiated using this factory...')
| if not os.path.isfile(script_path) or not script_path.endswith(".py"): | ||
| return None | ||
|
|
||
| return self._create_runtime(script_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, this should also have one of the three special functions defined.
This PR refactors the runtime architecture to support multiple entrypoints in a project and a pluggable runtime factory system using
Middlewares.register_runtime_factory()to retrieve a generic runtime factory and updates init, run, and eval to use the new runtime factory.The corresponding change in the langchain repo registers the runtime factory for langgraph scripts: UiPath/uipath-langchain-python#231