SEP-1655: Move the remaining Nomad-specific vocabulary out of app/tasks/models.py - #1226
Open
peter-o-addo wants to merge 4 commits into
Open
SEP-1655: Move the remaining Nomad-specific vocabulary out of app/tasks/models.py#1226peter-o-addo wants to merge 4 commits into
app/tasks/models.py#1226peter-o-addo wants to merge 4 commits into
Conversation
peter-o-addo
marked this pull request as ready for review
July 30, 2026 15:04
peter-o-addo
requested review from
a team,
marcuscruz-percona,
maxbube,
nachodd and
yyyyyyyan
as code owners
July 30, 2026 15:04
There was a problem hiding this comment.
Pull request overview
This PR continues the executor-neutralization work in the Tasks service by relocating remaining Nomad-specific vocabulary/constants out of the tasks models layer into the Nomad executor package, renaming Nomad-named log cursor columns/accessors to executor-neutral terms, and regenerating the API/TS client contract (with a documented breaking change for the renamed TaskHistory field).
Changes:
- Break the Nomad executor import cycle by switching
app.tasks.execution.executors.nomadto a lazyNomadExecutorexport and deferringapp.tasks.configimports where needed. - Rename Nomad-vocabulary log cursor columns to executor-neutral names (
nomad_offset→producer_fetch_offset,allocation_epoch→producer_epoch,log_allocation_epoch→log_producer_epoch) with a reversible Alembic migration and updated CRUD/log-writer logic. - Regenerate OpenAPI specs + TS clients and add a breaking changelog fragment for the
TaskHistoryresponse field rename; update tests accordingly (including new regression tests for the import-cycle and migration rename).
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/app/tasks/test_routes.py | Updates test imports to pull RUN_SCRIPT_OUTPUT_FILES_PATH from Nomad constants. |
| tests/app/tasks/migrations/test_taskhistory_log_nomad_cursor.py | Updates existing migration test expectations to use executor-neutral cursor column names. |
| tests/app/tasks/migrations/test_rename_log_cursor_columns.py | Adds coverage for the rename migration upgrade/downgrade preserving values. |
| tests/app/tasks/logs/test_log_writer.py | Updates log-writer tests to use new cursor/epoch parameter and field names. |
| tests/app/tasks/logs/test_log_eviction.py | Updates eviction test to pass the renamed epoch argument. |
| tests/app/tasks/execution/executors/nomad/test_models.py | Updates Nomad executor tests for renamed cursor/epoch fields. |
| tests/app/tasks/execution/executors/nomad/test_import_cycle.py | Adds a subprocess-based regression test ensuring Nomad submodules import without config-first ordering. |
| tests/app/tasks/db/test_seed.py | Updates seed tests to import Nomad-only task name from Nomad constants. |
| tests/app/sep/apps/framework/test_spec.py | Updates framework spec tests to import RUN_SCRIPT_OUTPUT_FILES_PATH from Nomad constants. |
| frontend/packages/api/src/generated/tasks.ts | Regenerates TS types/docs including log_producer_epoch and doc wording adjustments. |
| frontend/packages/api/src/generated/sep.ts | Regenerates shared TS types/docs for the same doc wording adjustments. |
| frontend/packages/api/specs/tasks.json | Regenerates OpenAPI spec reflecting log_producer_epoch and doc wording adjustments. |
| frontend/packages/api/specs/sep.json | Regenerates OpenAPI spec reflecting doc wording adjustments. |
| changelog.d/SEP-1630.breaking.md | Documents the breaking rename from log_allocation_epoch to log_producer_epoch. |
| app/tasks/models.py | Removes RUN_SCRIPT_OUTPUT_FILES_PATH from tasks models; renames cursor/epoch columns and neutralizes related docs; updates internal task-name set. |
| app/tasks/migrations/versions/2026_07_30_1300-c8e4a2b91f70_rename_log_cursor_columns_executor_neutral.py | Adds reversible Alembic column renames for executor-neutral naming. |
| app/tasks/logs/log_writer.py | Renames log-writer cursor/epoch args and guards; defers config import to avoid import cycles. |
| app/tasks/execution/executors/nomad/models.py | Updates Nomad executor log fetching/persisting logic to use executor-neutral cursor/epoch naming. |
| app/tasks/execution/executors/nomad/constants.py | Introduces a dependency-free Nomad constants module for layout and Nomad-only system task names. |
| app/tasks/execution/executors/nomad/init.py | Implements lazy NomadExecutor export via __getattr__ to break import cycles. |
| app/tasks/db/seed.py | Switches seed-time Nomad constants to the new Nomad constants module to avoid heavy imports. |
| app/tasks/crud.py | Renames TaskHistory epoch accessor/mutator methods and updates state-frontier persistence APIs to new field names. |
| app/sep/apps/framework/spec.py | Updates framework spec code to import RUN_SCRIPT_OUTPUT_FILES_PATH from Nomad constants. |
Comment on lines
236
to
240
| session: AsyncSession, | ||
| task_history_id: int, | ||
| *, | ||
| new_allocation_epoch: int, | ||
| new_producer_epoch: int, | ||
| ) -> None: |
Comment on lines
+774
to
777
| :return: ``(delta_text, new_producer_fetch_offset, new_producer_offset)`` — | ||
| the anonymized bytes fetched this cycle, the advanced Nomad-space | ||
| offset for the next fetch, and the advanced producer-space offset | ||
| that the writer should persist. |
Comment on lines
+993
to
1002
| #: Maintenance / system task names excluded from user-facing task lists. | ||
| #: The cert-expiry member is a literal matching | ||
| #: :data:`~app.tasks.execution.executors.nomad.constants.CHECK_NOMAD_CERT_EXPIRY_TASK_NAME` | ||
| #: so this module does not import the Nomad executor package. | ||
| INTERNAL_TASK_NAMES: frozenset[str] = frozenset( | ||
| { | ||
| INVENTORY_SYNC_TASK_NAME, | ||
| SYNC_RUNNING_TASKS_TASK_NAME, | ||
| CHECK_NOMAD_CERT_EXPIRY_TASK_NAME, | ||
| "tasks__check_nomad_cert_expiry", | ||
| } |
peter-o-addo
added
the
qa in progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move remaining Nomad-specific vocabulary out of the tasks models layer into the Nomad executor package, with executor-neutral log-cursor columns and a regenerated API contract.
app/tasks/execution/executors/nomad/__init__.py,app/tasks/logs/log_writer.py: break the Nomad package import cycle via lazy export and deferred config importsapp/tasks/execution/executors/nomad/constants.py,app/tasks/models.py,app/tasks/db/seed.py,app/sep/apps/framework/spec.py: relocate Nomad-owned constants and keep internal task names without importing the executor packageapp/tasks/models.py,app/tasks/crud.py,app/tasks/logs/log_writer.py,app/tasks/execution/executors/nomad/models.py: rename Nomad-named log-cursor columns/accessors to executor-neutral names and neutralize related docsapp/tasks/migrations/versions/2026_07_30_1300-c8e4a2b91f70_rename_log_cursor_columns_executor_neutral.py: rename those columns in place with a reversible Alembic migrationfrontend/packages/api/specs/{tasks,sep}.json,frontend/packages/api/src/generated/{tasks,sep}.ts,changelog.d/SEP-1630.breaking.md: regenerate OpenAPI/TS client for the TaskHistory field rename and record it as breakingTested
Checklist
make test)make run-pre-commit)make makemigrations)changelog.d/if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)