-
Notifications
You must be signed in to change notification settings - Fork 62
feat(runtime): add event_log_dir to configure event log output directory #474
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
base: main
Are you sure you want to change the base?
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -79,6 +79,7 @@ def __init__( | |||||||||||||
| *, | ||||||||||||||
| existing_path: Path | None = None, | ||||||||||||||
| existing_run_id: str | None = None, | ||||||||||||||
| event_log_dir: Path | None = None, | ||||||||||||||
|
Collaborator
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 Something like: The case this leaves confusing: set The module docstring at lines 3-4 also still says the log goes to |
||||||||||||||
| ) -> None: | ||||||||||||||
| """Initialise the subscriber. | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -150,11 +151,12 @@ def __init__( | |||||||||||||
| ) | ||||||||||||||
| self._run_id = new_run_id() | ||||||||||||||
| ts = time.strftime("%Y%m%d-%H%M%S") | ||||||||||||||
| self._path = ( | ||||||||||||||
| Path(tempfile.gettempdir()) | ||||||||||||||
| / "conductor" | ||||||||||||||
| / f"conductor-{workflow_name}-{ts}-{self._run_id}.events.jsonl" | ||||||||||||||
| base_dir = ( | ||||||||||||||
| event_log_dir | ||||||||||||||
| if event_log_dir is not None | ||||||||||||||
| else Path(tempfile.gettempdir()) / "conductor" | ||||||||||||||
| ) | ||||||||||||||
|
Comment on lines
+154
to
158
Collaborator
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. Minor: this collapses to one line.
Suggested change
That equivalence depends on the parameter staying |
||||||||||||||
| self._path = base_dir / f"conductor-{workflow_name}-{ts}-{self._run_id}.events.jsonl" | ||||||||||||||
|
Collaborator
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 deciding explicitly what should happen to a run whose log lands outside
I confirmed the History case: one entry from That last item has a user-visible consequence of its own. Either route these readers through a shared accessor, or say plainly in the field docstring that a custom directory opts the run out of History, retention, and background log correlation. The empty History screen is the part I would not leave silent. AGENTS.md is explicit that showing an empty state for a directory that was never read is a claim of absence that reads like success. |
||||||||||||||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||||||||||||||
| self._handle = open(self._path, "w", encoding="utf-8") # noqa: SIM115 | ||||||||||||||
|
Collaborator
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. Now that this path can come from user config, these two lines can raise. I checked both cases: pointing So a typo in a diagnostics setting now stops work that would otherwise have succeeded, which is the opposite of what an always-on flight recorder should do. Everything comparable in this codebase degrades instead. The Catching |
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| """Tests for event log directory path resolution.""" | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| from conductor.cli.run import _resolve_event_log_dir | ||
|
|
||
|
|
||
| def test_relative_event_log_dir_is_resolved_from_workflow_directory( | ||
| tmp_path: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Relative paths use the workflow file directory, not process CWD.""" | ||
| workflow_dir = tmp_path / "workflow" | ||
| workflow_dir.mkdir() | ||
| workflow_path = workflow_dir / "workflow.yaml" | ||
| workflow_path.write_text("", encoding="utf-8") | ||
|
|
||
| other_cwd = tmp_path / "elsewhere" | ||
| other_cwd.mkdir() | ||
| monkeypatch.chdir(other_cwd) | ||
|
|
||
| result = _resolve_event_log_dir("./logs", workflow_path) | ||
|
|
||
| assert result == (workflow_dir / "logs").resolve() | ||
| assert result != (other_cwd / "logs").resolve() | ||
|
|
||
|
|
||
| def test_parent_relative_event_log_dir_is_workflow_relative( | ||
| tmp_path: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Parent components are evaluated from the workflow directory.""" | ||
| workflow_dir = tmp_path / "project" / "workflows" | ||
| workflow_dir.mkdir(parents=True) | ||
| workflow_path = workflow_dir / "workflow.yaml" | ||
| workflow_path.write_text("", encoding="utf-8") | ||
|
|
||
| other_cwd = tmp_path / "elsewhere" | ||
| other_cwd.mkdir() | ||
| monkeypatch.chdir(other_cwd) | ||
|
|
||
| result = _resolve_event_log_dir("../logs", workflow_path) | ||
|
|
||
| assert result == (workflow_dir.parent / "logs").resolve() | ||
|
|
||
|
|
||
| def test_absolute_event_log_dir_is_unchanged( | ||
| tmp_path: Path, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| """Absolute paths do not depend on workflow location or process CWD.""" | ||
| workflow_dir = tmp_path / "workflow" | ||
| workflow_dir.mkdir() | ||
| workflow_path = workflow_dir / "workflow.yaml" | ||
| workflow_path.write_text("", encoding="utf-8") | ||
|
|
||
| other_cwd = tmp_path / "elsewhere" | ||
| other_cwd.mkdir() | ||
| monkeypatch.chdir(other_cwd) | ||
|
|
||
| absolute_dir = tmp_path / "absolute-logs" | ||
|
|
||
| assert _resolve_event_log_dir(str(absolute_dir), workflow_path) == absolute_dir.resolve() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("value", [None, ""]) | ||
| def test_missing_event_log_dir_remains_unset( | ||
| tmp_path: Path, | ||
| value: str | None, | ||
| ) -> None: | ||
| """Omitted and empty values retain the default TMPDIR behavior.""" | ||
| workflow_path = tmp_path / "workflow.yaml" | ||
| workflow_path.write_text("", encoding="utf-8") | ||
|
|
||
| assert _resolve_event_log_dir(value, workflow_path) is None |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||||||||
| """Tests for configurable event log output directory.""" | ||||||||||||
|
|
||||||||||||
| import tempfile | ||||||||||||
| import time | ||||||||||||
| from pathlib import Path | ||||||||||||
|
|
||||||||||||
| from conductor.engine.event_log import EventLogSubscriber | ||||||||||||
| from conductor.events import WorkflowEvent | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_runtime_config_event_log_dir(): | ||||||||||||
|
Collaborator
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. Every test in this file hands the subscriber a Cases worth adding: a relative value where the process runs somewhere other than the workflow file's directory, Two smaller notes. This test exercises Line 17 also has a stray pair of parens around the string literal. |
||||||||||||
| """RuntimeConfig accepts event_log_dir field, defaults to None.""" | ||||||||||||
| from conductor.config.schema import RuntimeConfig | ||||||||||||
|
|
||||||||||||
| assert RuntimeConfig().event_log_dir is None | ||||||||||||
| assert RuntimeConfig(event_log_dir="./logs").event_log_dir == "./logs" | ||||||||||||
| assert RuntimeConfig(event_log_dir="/var/log/conductor").event_log_dir == ("/var/log/conductor") | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_default_writes_to_tmpdir(): | ||||||||||||
| """Without event_log_dir, writes to $TMPDIR/conductor/ (existing behavior).""" | ||||||||||||
| sub = EventLogSubscriber("test_wf") | ||||||||||||
| try: | ||||||||||||
| assert sub.path.parent == Path(tempfile.gettempdir()) / "conductor" | ||||||||||||
| finally: | ||||||||||||
| sub.close() | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_custom_event_log_dir(tmp_path): | ||||||||||||
| """With event_log_dir, writes to the specified directory.""" | ||||||||||||
| sub = EventLogSubscriber( | ||||||||||||
| "test_wf", | ||||||||||||
| event_log_dir=tmp_path / "logs", | ||||||||||||
| ) | ||||||||||||
| try: | ||||||||||||
| assert sub.path.parent == tmp_path / "logs" | ||||||||||||
| assert sub.path.exists() | ||||||||||||
|
|
||||||||||||
| sub.on_event( | ||||||||||||
| WorkflowEvent( | ||||||||||||
| type="test", | ||||||||||||
| timestamp=time.time(), | ||||||||||||
| data={}, | ||||||||||||
| ) | ||||||||||||
| ) | ||||||||||||
| sub.close() | ||||||||||||
|
|
||||||||||||
| assert sub.path.read_text().strip() | ||||||||||||
| finally: | ||||||||||||
| if not sub._handle.closed: | ||||||||||||
| sub.close() | ||||||||||||
|
Comment on lines
+49
to
+51
Collaborator
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.
Suggested change
|
||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_existing_path_overrides_event_log_dir(tmp_path): | ||||||||||||
| """On resume, existing_path takes precedence over event_log_dir.""" | ||||||||||||
| existing = tmp_path / "existing.events.jsonl" | ||||||||||||
| existing.write_text("") | ||||||||||||
|
|
||||||||||||
| sub = EventLogSubscriber( | ||||||||||||
| "test_wf", | ||||||||||||
| existing_path=existing, | ||||||||||||
| existing_run_id="abcd1234", | ||||||||||||
| event_log_dir=tmp_path / "custom", | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| try: | ||||||||||||
| assert sub.path == existing | ||||||||||||
| finally: | ||||||||||||
| sub.close() | ||||||||||||
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.
The sentence about relative paths does not match
cli/run.py:2058, which resolves against the process working directory. See my note there. Whichever way that goes, this text has to agree with it. Worth notingspill_dira few hundred lines up has the same mechanics and documents the opposite rule, so right now two adjacent fields contradict each other.There is also no validator.
event_log_dir: " "is truthy, so it creates a directory named with three spaces.""happens to behave, but only becauserun.pytests truthiness whileevent_log.py:155testsis not None. That split is what_normalize_spill_dirat line 3123 exists to prevent, and its docstring says so directly.Two things the docstring should also say: a resumed run keeps writing to the log recorded in its checkpoint and ignores this setting, and a log written outside
$TMPDIR/conductor/is skipped by the retention sweep and byconductor fleetHistory.grep event_log_dir docs/currently comes back empty, anddocs/cli-reference.md:333still states the log lives under$TMPDIR/conductor/.