-
Notifications
You must be signed in to change notification settings - Fork 46
feat: accept and forward resolvedSymbolTable to the Rust session runtime #1063
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 |
|---|---|---|
|
|
@@ -1227,6 +1227,8 @@ def _create_new_sessions( | |
| region=self._boto_session.region_name, | ||
| ) | ||
|
|
||
| resolved_symbol_table_json = queue.peek_resolved_symbol_table_json() | ||
|
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. This peek performs a blocking The docstring on the new accessor says "Entity resolution results are cached by Two consequences:
Since |
||
|
|
||
| try: | ||
| session = Session( | ||
| id=new_session_id, | ||
|
|
@@ -1244,6 +1246,7 @@ def _create_new_sessions( | |
| session_root_dir=self._session_root_dir, | ||
| farm_id=self._farm_id, | ||
| region=self._boto_session.region_name, | ||
| resolved_symbol_table_json=resolved_symbol_table_json, | ||
| ) | ||
| except (ValueError, NotImplementedError, OSError) as e: | ||
| # Runtime construction can fail per-session (e.g. the selected runtime's | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -452,6 +452,7 @@ def dequeue(self) -> SessionActionDefinition | None: | |
| next_action = ExitEnvironmentAction( | ||
| id=action_id, | ||
| environment_id=environment_id, | ||
| details=environment_details, | ||
| ) | ||
| else: | ||
| raise ValueError(f'Unknown action type "{action_type}".') | ||
|
|
@@ -557,3 +558,51 @@ def dequeue(self) -> SessionActionDefinition | None: | |
| f'Unknown action type "{action_type}". Complete action = {action_definition}' | ||
| ) | ||
| return next_action | ||
|
|
||
| def peek_resolved_symbol_table_json(self) -> str | None: | ||
| """Inspect the first queued action's resolved symbol table without consuming it. | ||
|
|
||
| This accessor is non-consuming: the queue state is not mutated, and a | ||
| subsequent ``dequeue`` call will still yield the same front action. | ||
|
|
||
| Entity resolution results are cached by ``JobEntities``, so the later | ||
|
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. Two things about this docstring claim are worth correcting, because both invite a wrong conclusion at the call site. "Entity resolution results are cached by "a failure must not break session creation. The subsequent dequeue surfaces the real error through the normal action-failure path." The second sentence is only true for the terminal-error case. If the peek failed for a transient reason and the dequeue retry succeeds, no error is ever surfaced — the session runs to completion with Also, the warning drops the exception entirely. Given that this swallows |
||
| ``dequeue`` issues no additional service request for the same entity. | ||
|
|
||
| Returns | ||
| ------- | ||
| str | None | ||
| The ``resolved_symbol_table_json`` from the first action's entity, | ||
| or None when the queue is empty or the action type has no table. | ||
| """ | ||
| if not self._actions: | ||
|
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. Peeking only The The result is a feature that works or does not work based on action ordering, with no signal either way: the
At minimum, worth a |
||
| return None | ||
|
|
||
| action_queue_entry = self._actions[0] | ||
| action_type = action_queue_entry.definition["actionType"] | ||
|
|
||
| try: | ||
| if action_type.startswith("ENV_"): | ||
| action_queue_entry = cast(EnvironmentQueueEntry, action_queue_entry) | ||
| environment_id = action_queue_entry.definition["environmentId"] | ||
| environment_details = self._job_entities.environment_details( | ||
| environment_id=environment_id | ||
| ) | ||
| return environment_details.resolved_symbol_table_json | ||
| elif action_type == "TASK_RUN": | ||
| action_queue_entry = cast(TaskRunQueueEntry, action_queue_entry) | ||
| step_id = action_queue_entry.definition["stepId"] | ||
| step_details = self._job_entities.step_details(step_id=step_id) | ||
| return step_details.resolved_symbol_table_json | ||
| else: | ||
| return None | ||
| except Exception: | ||
| # This accessor only seeds session-scoped symbols (e.g. Job.Name), | ||
| # so a failure must not break session creation. The subsequent | ||
| # dequeue surfaces the real error through the normal action-failure | ||
| # path. | ||
| logger.warning( | ||
| "Failed to prefetch resolved symbol table for the first queued action " | ||
| "(type=%s); proceeding without it.", | ||
| action_type, | ||
| ) | ||
| return None | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,4 +109,5 @@ def start( | |
| job_env_id=self._job_env_id, | ||
| environment=self._details.environment, | ||
| os_env_vars={"DEADLINE_SESSIONACTION_ID": self._id}, | ||
| resolved_symbol_table_json=self._details.resolved_symbol_table_json, | ||
|
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.
Worth confirming which is true: if the session caches it, a brief comment saying so would prevent a future reader from “fixing” this; if it does not, |
||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| from .openjd_action import OpenjdAction | ||
|
|
||
| if TYPE_CHECKING: | ||
| from ..job_entities import EnvironmentDetails | ||
| from ..session import Session | ||
|
|
||
|
|
||
|
|
@@ -20,26 +21,32 @@ class ExitEnvironmentAction(OpenjdAction): | |
| A unique identifier for the session action | ||
| environment_id : str | ||
| The job environment identifier | ||
| details : EnvironmentDetails | None | ||
| Optional environment details carrying the pre-resolved symbol table | ||
| """ | ||
|
|
||
| _environment_id: str | ||
| _details: EnvironmentDetails | None | ||
|
|
||
| def __init__( | ||
| self, | ||
| *, | ||
| id: str, | ||
| environment_id: str, | ||
| details: EnvironmentDetails | None = None, | ||
|
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. Making There is exactly one non-test construction site — session_queue.py:452 — and by that point The cost of the default is that the failure is silent. Forget the kwarg at some future call site and Given If the optionality is deliberate — e.g. anticipating a future caller that genuinely has no details — a comment naming that caller would help, since today the signature suggests a case that does not exist. |
||
| ) -> None: | ||
| super(ExitEnvironmentAction, self).__init__( | ||
| id=id, action_log_kind=SessionActionLogKind.ENV_EXIT | ||
| ) | ||
| self._environment_id = environment_id | ||
| self._details = details | ||
|
|
||
| def __eq__(self, other: Any) -> bool: | ||
| return ( | ||
| type(self) is type(other) | ||
| and self._id == other._id | ||
| and self._environment_id == other._environment_id | ||
| and self._details == other._details | ||
| ) | ||
|
|
||
| def start( | ||
|
|
@@ -58,5 +65,9 @@ def start( | |
| An executor for running futures | ||
| """ | ||
| session.exit_environment( | ||
| job_env_id=self._environment_id, os_env_vars={"DEADLINE_SESSIONACTION_ID": self._id} | ||
| job_env_id=self._environment_id, | ||
| os_env_vars={"DEADLINE_SESSIONACTION_ID": self._id}, | ||
| resolved_symbol_table_json=self._details.resolved_symbol_table_json | ||
| if self._details is not None | ||
| else None, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,4 +85,5 @@ def start(self, *, session: Session, executor: Executor) -> None: | |
| task_parameter_values=self._task_parameter_values, | ||
| os_env_vars=env_vars, | ||
| step_name=self._details.step_template.name, | ||
| resolved_symbol_table_json=self._details.resolved_symbol_table_json, | ||
|
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. Scope mismatch worth confirming: the resolved table arrives on stepDetails, but it is consumed once per task.
That is fine if the service only ever puts step-and-above-scoped symbols in it ( Worth confirming which contract the service guarantees. If task-scoped symbols are excluded by design, a short comment here saying the table is step-scoped would protect the invariant; if they are not, the table needs to be per-task rather than carried on the cached step entity. |
||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,9 @@ class StepDetails: | |
| dependencies: list[str] = field(default_factory=list) | ||
| """The dependencies (a list of IDs) that the step depends on""" | ||
|
|
||
| resolved_symbol_table_json: str | None = None | ||
| """Pre-resolved symbol table JSON from the service, forwarded to the Rust session runtime.""" | ||
|
|
||
| @classmethod | ||
| def from_boto(cls, step_details_data: StepDetailsData) -> StepDetails: | ||
| """Converts an stepDetails entity received from BatchGetJobEntity API response into a | ||
|
|
@@ -86,6 +89,7 @@ def from_boto(cls, step_details_data: StepDetailsData) -> StepDetails: | |
| step_template=step_template, | ||
| step_id=step_details_data["stepId"], | ||
| dependencies=step_details_data["dependencies"], | ||
| resolved_symbol_table_json=step_details_data.get("resolvedSymbolTable", None), | ||
| ) | ||
|
|
||
| @classmethod | ||
|
|
@@ -118,6 +122,7 @@ def validate_entity_data(cls, entity_data: dict[str, Any]) -> StepDetailsData: | |
| Field(key="template", expected_type=dict, required=True), | ||
| Field(key="stepId", expected_type=str, required=True), | ||
| Field(key="dependencies", expected_type=list, required=False), | ||
| Field(key="resolvedSymbolTable", expected_type=str, required=False), | ||
|
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. This
The gap is that the Two cheap parametrize cases would close it, in both this file and
The second is worth having on its own: |
||
| Field(key="extensions", expected_type=list, required=False), | ||
| ), | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,3 +37,9 @@ class SessionRuntimeConfig: | |
| session_root_directory: Path | ||
| spec_revision: str = "2023-09" | ||
| supported_extensions: tuple[str, ...] = () | ||
| resolved_symbol_table_json: str | None = None | ||
|
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.
That makes the config-level plumbing dead weight on the path the feature is actually built for. Because So the cost lands on precisely the sessions that do not benefit, while the sessions that do benefit ( Cheapest fix is to gate the peek on the selected kind, since resolved_symbol_table_json = (
queue.peek_resolved_symbol_table_json()
if runtime_kind is SessionRuntimeKind.PYTHON
else None
)That removes the extra request from the Rust path entirely, and makes the narrow purpose of this field — which this docstring already describes accurately as "Adapters that seed session-scoped symbols at construction (e.g. job_name for the classic Python session)" — visible at the call site too. |
||
| """Pre-resolved symbol table JSON observed on the session's first queued action. | ||
|
|
||
| Adapters that seed session-scoped symbols at construction (e.g. job_name for | ||
| the classic Python session) consume this at build time. | ||
| """ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ | |
| from __future__ import annotations | ||
|
|
||
| from datetime import timedelta | ||
| from logging import getLogger | ||
| from pathlib import Path | ||
| from typing import TYPE_CHECKING, Any, Optional | ||
|
|
||
| from openjd.expr import SerializedSymbolTable | ||
|
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. This module-level
That guard exists for
So the fallback path now inherits the exact failure mode it was structured to survive, and it does so to obtain one optional display string. Worth confirming which is actually true here, since the fix differs:
Note the new tests cannot distinguish these: |
||
| from openjd.model import RevisionExtensions, SpecificationRevision | ||
| from openjd.sessions import Session as OpenJDSession | ||
|
|
||
|
|
@@ -22,6 +24,29 @@ | |
|
|
||
| __all__ = ["PythonSessionRuntime"] | ||
|
|
||
| logger = getLogger(__name__) | ||
|
|
||
|
|
||
| def _extract_job_name(json_str: str | None) -> str | None: | ||
| """Extract the Job.Name value from a resolved symbol table JSON string. | ||
|
|
||
| Returns None when the input is None, the table lacks a Job.Name entry, the | ||
| entry's value is not a string, or parsing fails (graceful degradation — | ||
| mirrors _parse_resolved_symtab in the Rust adapter). | ||
| """ | ||
| if json_str is None: | ||
| return None | ||
| try: | ||
| symtab = SerializedSymbolTable.from_json_str(json_str).to_symtab() | ||
| entry = symtab.get("Job.Name") | ||
| if entry is None: | ||
| return None | ||
| value = entry.item() | ||
| return value if isinstance(value, str) else None | ||
| except Exception as e: | ||
|
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. This The table holds resolved job/task parameter values — the same data What makes this line riskier than
Because Suggest logging only 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. This
This one is not, and it cannot be:
The trigger is service-controlled and is exactly the payload class the graceful-degradation story is meant to absorb: well-formed JSON whose entries are not valid symbol-table entries. Two options:
Worth noting the current tests cannot see this: |
||
| logger.warning("Failed to extract Job.Name from resolvedSymbolTable: %s", e) | ||
| return None | ||
|
|
||
|
|
||
| class PythonSessionRuntime(SessionRuntime): | ||
| """SessionRuntime backed by openjd.sessions (v0 Python implementation).""" | ||
|
|
@@ -38,6 +63,7 @@ def __init__(self, config: SessionRuntimeConfig) -> None: | |
| callback=config.action_callback, | ||
| os_env_vars=config.os_env_vars, | ||
| session_root_directory=config.session_root_directory, | ||
| job_name=_extract_job_name(config.resolved_symbol_table_json), | ||
|
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. Sourcing
Given the peek already parses the table anyway, a length cap and a rejection of control characters (or at minimum a comment recording that upstream |
||
| revision_extensions=RevisionExtensions( | ||
| # Currently for simplicity request that our session allow all extensions. | ||
| # This does not obey the spec. It should be changed at a later date to the | ||
|
|
@@ -53,7 +79,10 @@ def enter_environment( | |
| environment: EnvironmentModel, | ||
| identifier: Optional[EnvironmentIdentifier] = None, | ||
| os_env_vars: Optional[dict[str, str]] = None, | ||
| resolved_symbol_table_json: str | None = None, | ||
| ) -> EnvironmentIdentifier: | ||
| # resolved_symbol_table_json: not forwarded — the v0 Python session does | ||
| # not support pre-resolved symbol tables. | ||
|
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. The Python (v0) runtime accepts |
||
| return self._session.enter_environment( | ||
| environment=environment, | ||
| identifier=identifier, | ||
|
|
@@ -66,7 +95,10 @@ def exit_environment( | |
| identifier: EnvironmentIdentifier, | ||
| os_env_vars: Optional[dict[str, str]] = None, | ||
| keep_session_running: bool = False, | ||
| resolved_symbol_table_json: str | None = None, | ||
| ) -> None: | ||
| # resolved_symbol_table_json: not forwarded — the v0 Python session does | ||
| # not support pre-resolved symbol tables. | ||
| self._session.exit_environment( | ||
| identifier=identifier, | ||
| os_env_vars=os_env_vars, | ||
|
|
@@ -81,7 +113,10 @@ def run_task( | |
| os_env_vars: Optional[dict[str, str]] = None, | ||
| log_task_banner: bool = True, | ||
| step_name: str | None = None, | ||
| resolved_symbol_table_json: str | None = None, | ||
| ) -> None: | ||
| # resolved_symbol_table_json: not forwarded — the v0 Python session does | ||
| # not support pre-resolved symbol tables. | ||
| self._session.run_task( | ||
| step_script=step_script, | ||
| task_parameter_values=task_parameter_values, | ||
|
|
||
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.
New wire field, but
session_events.pyis untouched — worth checking whether that is the intended outcome for the agent log.LOGGING_ALLOW_LIST["deadline:BatchGetJobEntity"]["res_log_body"](session_events.py:161) enumerates the loggablestepDetails/environmentDetailskeys, and_get_loggable_parameters(session_events.py:206) replaces any key not in that map with*REDACTED*. SinceresolvedSymbolTablewas not added there, it currently logs asresolvedSymbolTable: "*REDACTED*"— which is the safe default, and correct given the table holds resolved job/task parameter values (the same reasoning the existing comments give for excludingtemplateandjobDetails.parameters).So no leak today. The ask is just to make it deliberate rather than incidental: adding an explicit
"resolvedSymbolTable": False, # resolved parameter values; sensitiveentry alongside the existing exclusions documents the decision and prevents a future"*": True-style change or allow-list refactor from silently opting this field in.