-
Notifications
You must be signed in to change notification settings - Fork 20
feat: Add run_subprocess function to the Session class #282
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,13 @@ | |
| ) | ||
| from openjd.model import version as model_version | ||
| from openjd.model.v2023_09 import ( | ||
| Action as Action_2023_09, | ||
| ArgString as ArgString_2023_09, | ||
| CancelationMethodTerminate as CancelationMethodTerminate_2023_09, | ||
| CancelationMode as CancelationMode_2023_09, | ||
| CommandString as CommandString_2023_09, | ||
| StepActions as StepActions_2023_09, | ||
| StepScript as StepScript_2023_09, | ||
| ValueReferenceConstants as ValueReferenceConstants_2023_09, | ||
| ) | ||
| from ._action_filter import ActionMessageKind, ActionMonitoringFilter | ||
|
|
@@ -900,6 +907,122 @@ def _run_task_without_session_env( | |
| # than after -- run() itself may end up setting the action state to FAILED. | ||
| self._runner.run() | ||
|
|
||
| def run_subprocess( | ||
| self, | ||
| *, | ||
| command: str, | ||
| args: Optional[list[str]] = None, | ||
| timeout: Optional[int] = None, | ||
| os_env_vars: Optional[dict[str, str]] = None, | ||
| use_session_env_vars: bool = True, | ||
| log_banner_message: Optional[str] = None, | ||
| ) -> None: | ||
| """Run an ad-hoc subprocess within the Session. | ||
|
|
||
| This method is non-blocking; it will exit when the subprocess is either | ||
| confirmed to have started running, or has failed to be started. | ||
|
|
||
| Arguments: | ||
| command (str): The command/executable to run. Used exactly as provided | ||
| without format string substitution. | ||
| args (Optional[list[str]]): Arguments to pass to the command. Used exactly | ||
| as provided without format string substitution. Defaults to None. | ||
| timeout (Optional[int]): Maximum allowed runtime of the subprocess in seconds. | ||
| Must be a positive integer if provided. If None, the subprocess can run | ||
| indefinitely. Defaults to None. | ||
| os_env_vars (Optional[dict[str, str]]): Additional OS environment variables | ||
| to inject into the subprocess. Values provided override original process | ||
| environment variables and are overridden by environment-defined variables. | ||
| use_session_env_vars (bool): If True, includes environment variables from | ||
| the session and entered environments. If False, only uses os_env_vars | ||
| and original process environment variables. Defaults to True. | ||
| log_banner_message (Optional[str]): Custom message to display in a banner | ||
| before running the subprocess. If provided, logs a banner with this message. | ||
| If None, no banner is logged. Defaults to None. | ||
|
|
||
| Raises: | ||
| RuntimeError: If the Session is not in the READY state. | ||
| ValueError: If timeout is provided and is not a positive integer, or if command is empty. | ||
| """ | ||
| # State validation | ||
| if self.state != SessionState.READY: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not allow RUNNING? If we want to run multiple subprocesses using this new method, it looks like the session would be in RUNNING after the first call and the second call would fail.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing as for the non-blocking part. |
||
| raise RuntimeError( | ||
| f"Session must be in the READY state to run a subprocess. " | ||
| f"Current state: {self.state.value}" | ||
| ) | ||
|
|
||
| # Parameter validation | ||
| if timeout is not None and timeout <= 0: | ||
| raise ValueError("timeout must be a positive integer") | ||
|
|
||
| if not command or not command.strip(): | ||
| raise ValueError("command must be a non-empty string") | ||
|
|
||
| # Log banner if requested | ||
| if log_banner_message: | ||
| log_section_banner(self._logger, log_banner_message) | ||
|
|
||
| # Reset action state | ||
| self._reset_action_state() | ||
|
|
||
| # Construct Action model | ||
| cancelation = CancelationMethodTerminate_2023_09(mode=CancelationMode_2023_09.TERMINATE) | ||
|
|
||
| action_command = CommandString_2023_09(command) | ||
| action_args = [ArgString_2023_09(arg) for arg in args] if args else None | ||
|
|
||
| action = Action_2023_09( | ||
| command=action_command, | ||
| args=action_args, | ||
| timeout=timeout, | ||
| cancelation=cancelation, | ||
| ) | ||
|
|
||
| # Construct StepScript model | ||
| step_actions = StepActions_2023_09(onRun=action) | ||
|
|
||
| step_script = StepScript_2023_09( | ||
| actions=step_actions, | ||
| embeddedFiles=None, | ||
| ) | ||
|
|
||
| # Create empty symbol table (no format string substitution for ad-hoc subprocesses) | ||
| symtab = SymbolTable() | ||
|
|
||
| # Evaluate environment variables | ||
| if use_session_env_vars: | ||
| action_env_vars = self._evaluate_current_session_env_vars(os_env_vars) | ||
| else: | ||
| action_env_vars = dict[str, Optional[str]](self._process_env) # Make a copy | ||
| if os_env_vars: | ||
| action_env_vars.update(**os_env_vars) | ||
|
|
||
| # Note: Path mapping is not materialized for ad-hoc subprocesses since it's only | ||
| # accessible via template variable substitution (e.g., {{Session.PathMappingRulesFile}}), | ||
| # which is explicitly disabled for run_subprocess to ensure predictable behavior. | ||
|
|
||
| # Create and start StepScriptRunner | ||
| self._runner = StepScriptRunner( | ||
| logger=self._logger, | ||
| user=self._user, | ||
| os_env_vars=action_env_vars, | ||
| session_working_directory=self.working_directory, | ||
| startup_directory=self.working_directory, | ||
| callback=self._action_callback, | ||
| script=step_script, | ||
| symtab=symtab, | ||
| session_files_directory=self.files_directory, | ||
| ) | ||
|
|
||
| # Sets the subprocess running. | ||
| # Returns immediately after it has started, or is running | ||
| self._action_state = ActionState.RUNNING | ||
| self._state = SessionState.RUNNING | ||
| # Note: This may fail immediately (e.g. if we cannot write embedded files to disk), | ||
| # so it's important to set the action_state to RUNNING before calling run(), rather | ||
| # than after -- run() itself may end up setting the action state to FAILED. | ||
| self._runner.run() | ||
|
|
||
| # ========================= | ||
| # Helpers | ||
|
|
||
|
|
||
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.
Would it make sense to make
blockingan parameter? Maybe not, I'm not quite what the intended use case is.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 behavior fits
run_subprocessinto the sequential actions that are part of a session, just like the existingrun_taskorenter_environment. Expanding the environment to run in parallel to other actions makes sense, but it would need more verification that the code is compatible with that concurrency, compared to fitting in the existing patterns the way it is now. Because of that, I'd rather wait until we have that use case to expand it.