diff --git a/CHANGELOG.md b/CHANGELOG.md index ab53a8a..89294a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ All notable changes to RushTI are documented in this file. +## Unreleased + +- **Per-workflow `tm1_instance` setting** for results push and auto-load. + Set it inside a JSON taskfile's `settings` block to override the + `settings.ini` default per workflow. Resolution chain (highest wins): + CLI `--tm1-instance` > taskfile `settings.tm1_instance` > + `settings.ini [tm1_integration].tm1_instance` > + `settings.ini [tm1_integration].default_tm1_instance` (deprecated). + RushTI now logs which tier supplied the target at push time. +- **Behavioural change:** `push_results` and `auto_load_results` set + inside taskfile JSON now take effect. They were silently ignored in + the `run` path before this release. If you have divergent values + between taskfile JSON and `settings.ini`, reconcile them before + upgrading — the taskfile value will now win. +- **Soft deprecation:** `settings.ini [tm1_integration].default_tm1_instance`. + Rename to `tm1_instance`. The old key is honoured indefinitely as the + final fallback in the resolution chain; a one-shot `DEPRECATION:` + warning fires at settings load only when it's the value actually being + used. +- **Soft deprecation:** `rushti tasks push --target-tm1-instance`. Use + `--tm1-instance` instead. The legacy flag is aliased and continues to + work; a `DEPRECATION:` warning fires on use. + ## Unreleased — `feat/issue-146-detailed-results` - Add `--detailed-results` for per-execution cube rows (closes #146). diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 0000000..a5ac4dd --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,106 @@ +# RushTI — Domain Context + +This file captures the canonical vocabulary used in RushTI. Terms here are +load-bearing: they appear in code, docs, settings, and user-facing messages. +When a term feels ambiguous in conversation, anchor it to a definition here. + +> Scope: single context. No `CONTEXT-MAP.md` — the whole repository shares +> this vocabulary. + +--- + +## Core terms + +### Taskfile +The structured input that drives a `rushti run`. Materialised as either: +- a **JSON task file** (`.json`) parsed by `parse_json_taskfile`, OR +- a **TXT task file** (`.txt`) converted on read by `convert_txt_to_json`, OR +- a **TM1 cube taskfile** read by `read_taskfile_from_tm1` (an MDX query + against the `rushti` cube using the `Input` element of `rushti_run_id`). + +The in-memory shape is the `Taskfile` dataclass (`taskfile.py`) — a container +with `version`, `metadata`, `settings`, and `tasks`. Always prefer +**"taskfile"** (one word, lowercase) in docs; **"JSON task file"** is also +acceptable when the format matters. + +Do **not** call it `workflow.json` — that name implies the file *is* a +workflow, which conflates the container with one of its metadata fields. + +### Workflow +The logical identifier for a run, stored in `metadata.workflow` of a taskfile +and as an element of the `rushti_workflow` dimension. A taskfile contains +tasks *for* one workflow. + +Use **"workflow"** as an adjective for scope ("workflow-level setting", +"per-workflow override") rather than as a synonym for "taskfile". + +### Task +A single TI process execution inside a taskfile. Each task has: +- `id` — a positive integer string, used as an element name in + `rushti_task_id`. +- `instance` — **the TM1 instance where this task executes** (task-level). +- `process` — the TI process name on that instance. +- `parameters`, `predecessors`, `stage`, `timeout`, etc. + +### TM1 instance +A named TM1 server defined in `config.ini`. Three roles can apply +contextually — context disambiguates, not the name: + +| Role | Meaning | Where it shows up | +|---|---|---| +| **Source** | Where a taskfile is read *from* (cube source) | `rushti run --tm1-instance X` | +| **Execution target** | Where a task executes | task-level `instance` field | +| **Results target** | Where execution results are pushed | settings.ini or taskfile `tm1_instance`; also CLI `--tm1-instance` as fallback | + +The canonical setting key for the results target is `tm1_instance` (in both +`settings.ini [tm1_integration]` and the taskfile `settings` block). The +deprecated alias is `default_tm1_instance`. + +There is no separate "source TM1 instance" override — `--tm1-instance` is +both the source and (in absence of higher-precedence overrides) the results +target for a `run` invocation. + +### Source vs target — naming convention +RushTI does not prefix instance names with `source_` or `target_`. The +*command* or the *config section* makes the role obvious: +- `rushti run --tm1-instance X` — X is the source for cube-read, and the + fallback results target if nothing else is set. +- `rushti tasks push --tm1-instance X` — X is the target of the push + (canonical form). The legacy alias `--target-tm1-instance` is deprecated. +- `[tm1_integration].tm1_instance` — the section header makes "target" clear. + +This convention is intentional: prefixes don't scale across surfaces and +fight against contextual disambiguation. See +[[adr/0001-tm1-instance-resolution]] for the full rationale. + +--- + +## Settings precedence + +The effective value for a settings-driven knob is resolved in this order +(highest wins): + +1. **CLI arguments** — e.g. `--max-workers`, `--tm1-instance`. +2. **Taskfile settings block** — e.g. `{"settings": {"max_workers": 8}}` + in JSON. Applied via `_apply_json_settings` inside + `get_effective_settings` *after* the taskfile is parsed. +3. **`settings.ini`** — the canonical defaults file at + `config/settings.ini`. +4. **Built-in defaults** — the dataclass field defaults in + `settings.py`. + +Three knobs are *not* settings-driven and don't follow this chain: +- Per-task `instance` and `process` — taskfile-only, no fallback. +- TM1 connection parameters — `config.ini` only. + +--- + +## Files of record + +| File | Purpose | +|---|---| +| `config/settings.ini` | Execution defaults (user-editable). | +| `config/settings.ini.template` | Documented example, shipped in repo. | +| `config/config.ini` | TM1 connection parameters per instance. | +| `archive/{workflow}/{run_id}.json` | Snapshot of the taskfile actually executed, for audit + DAG reconstruction. | +| `data/rushti_stats.db` | Local SQLite stats database (when `[stats] enabled = true`). | diff --git a/config/settings.ini.template b/config/settings.ini.template index 284e0cd..f3b1bb9 100644 --- a/config/settings.ini.template +++ b/config/settings.ini.template @@ -113,7 +113,7 @@ # To set up TM1 integration: # 1. Run: python rushti.py build --tm1-instance tm1srv01 # This creates the required dimensions and cube automatically. -# 2. Set push_results = true and configure default_tm1_instance below +# 2. Set push_results = true and configure tm1_instance below # # Default: false # push_results = false @@ -126,9 +126,18 @@ # Default: false # auto_load_results = false -# Default TM1 instance for reading taskfiles and writing results -# Used when --tm1-instance is specified without an instance, and for auto-upload -# Must be defined in config.ini +# TM1 instance used as the results-push target when no higher-precedence +# override is set. Resolved against a 4-tier chain (highest wins): +# 1. CLI --tm1-instance +# 2. Taskfile JSON settings.tm1_instance +# 3. This key +# 4. default_tm1_instance below (deprecated) +# Must be defined in config.ini. +# tm1_instance = tm1srv01 + +# DEPRECATED: use tm1_instance above. Still honoured indefinitely as the +# final fallback; a one-shot DEPRECATION warning fires at settings load +# *only* when tm1_instance is unset and this key is the value being used. # default_tm1_instance = tm1srv01 # Default cube name for task definitions and results diff --git a/docs/advanced/cli-reference.md b/docs/advanced/cli-reference.md index e54e969..6a14ddb 100644 --- a/docs/advanced/cli-reference.md +++ b/docs/advanced/cli-reference.md @@ -235,8 +235,8 @@ rushti tasks push --tasks daily-etl.json --tm1-instance tm1srv01 | Option | Short | Type | Description | |--------|-------|------|-------------| | `--tasks` | `-t` | PATH | Local JSON task file to push (*required*) | -| `--tm1-instance` | | STR | Source TM1 instance (if loading from TM1) | -| `--target-tm1-instance` | | STR | Target TM1 instance for the push | +| `--tm1-instance` | | STR | TM1 instance to push the taskfile to. Context disambiguates the role — on `tasks push` this is the destination. | +| `--target-tm1-instance` | | STR | **Deprecated alias** for `--tm1-instance`. Still works; emits a `DEPRECATION:` warning when used. | | `--settings` | `-s` | PATH | Path to `settings.ini` | --- diff --git a/docs/advanced/settings-reference.md b/docs/advanced/settings-reference.md index e232efa..c43ea87 100644 --- a/docs/advanced/settings-reference.md +++ b/docs/advanced/settings-reference.md @@ -120,7 +120,8 @@ Controls TM1-based read/write integration: reading task files from a TM1 cube an | `push_results` | bool | `false` | Upload the results CSV to the TM1 Applications folder after each run. The file is named `rushti_{workflow}_{run_id}.csv` (with `.blb` extension for TM1 < v12). | | `auto_load_results` | bool | `false` | After uploading results (requires `push_results = true`), call the `}rushti.load.results` TI process on the target TM1 instance to load the CSV into the rushti cube. Passes `pSourceFile` and `pTargetCube` parameters. The process must exist on the target instance. | | `detailed_results` | bool | `false` | Emit one cube row per executed TI when pushing results, instead of summarizing expanded tasks (only meaningful with `push_results = true`). Each row gets a fresh sequential `task_id`; the original IDs are preserved in the new `original_task_id` measure. See [TM1 integration: detailed results](../features/tm1-integration.md#detailed-results). | -| `default_tm1_instance` | str | *(none)* | Default TM1 instance name (from `config.ini`) used for reading task files and writing results. Required when `push_results` is enabled. | +| `tm1_instance` | str | *(none)* | TM1 instance (from `config.ini`) used as the **results-push target** when no higher-precedence override is set. Resolved per [the 4-tier chain](../features/tm1-integration.md#per-workflow-target-instance): CLI `--tm1-instance` > taskfile `settings.tm1_instance` > this key > `default_tm1_instance` (deprecated). Required when `push_results` is enabled and no higher tier is set. | +| `default_tm1_instance` | str | *(none)* | **Deprecated** — use `tm1_instance`. Honoured indefinitely as the final fallback in the resolution chain; a one-shot `DEPRECATION:` warning fires at settings load *only* when it's actually being used (i.e. `tm1_instance` is unset). | | `default_rushti_cube` | str | `rushti` | Name of the TM1 cube for task definitions and execution results. Created by the `rushti build` command. | | `default_workflow_dim` | str | `rushti_workflow` | Dimension name for workflow identifiers. | | `default_task_id_dim` | str | `rushti_task_id` | Dimension name for task sequence elements (1--5000 default elements). | @@ -131,7 +132,7 @@ Controls TM1-based read/write integration: reading task files from a TM1 cube an **Overridable via CLI:** `--detailed-results` -**Overridable via JSON task file:** `push_results`, `auto_load_results`, `detailed_results` +**Overridable via JSON task file:** `push_results`, `auto_load_results`, `detailed_results`, `tm1_instance` !!! info "Setting Up TM1 Integration" Run `rushti build --tm1-instance ` to create the required dimensions and cube automatically before enabling `push_results`. @@ -293,7 +294,7 @@ Copy this template to `config/settings.ini` and uncomment the settings you want # To set up TM1 integration: # 1. Run: rushti build --tm1-instance tm1srv01 # This creates the required dimensions and cube automatically. -# 2. Set push_results = true and configure default_tm1_instance below +# 2. Set push_results = true and configure tm1_instance below # # Default: false # push_results = false @@ -308,6 +309,11 @@ Copy this template to `config/settings.ini` and uncomment the settings you want # Default TM1 instance for reading taskfiles and writing results # Must be defined in config.ini +# tm1_instance = tm1srv01 + +# DEPRECATED: use tm1_instance above. Honoured indefinitely as the final +# fallback in the resolution chain; a one-shot DEPRECATION warning fires +# only when this key is the value actually being used. # default_tm1_instance = tm1srv01 # Default cube name for task definitions and results diff --git a/docs/features/tm1-integration.md b/docs/features/tm1-integration.md index 6c576dc..3d9caca 100644 --- a/docs/features/tm1-integration.md +++ b/docs/features/tm1-integration.md @@ -148,7 +148,7 @@ retention_days = 90 [tm1_integration] push_results = true -default_tm1_instance = tm1srv01 +tm1_instance = tm1srv01 default_rushti_cube = rushti ``` @@ -163,7 +163,7 @@ To automatically load the CSV into the cube after each push, enable `auto_load_r [tm1_integration] push_results = true auto_load_results = true -default_tm1_instance = tm1srv01 +tm1_instance = tm1srv01 default_rushti_cube = rushti ``` @@ -198,7 +198,8 @@ All TM1 integration settings live in the `[tm1_integration]` section of `config/ |---------|---------|-------------| | `push_results` | `false` | Push a results CSV to TM1 after each run | | `auto_load_results` | `false` | Automatically call `}rushti.load.results` to load the CSV into the cube | -| `default_tm1_instance` | *(none)* | TM1 instance to use for reading/writing cube data | +| `tm1_instance` | *(none)* | Default results-push target. Resolved against the [4-tier chain](#per-workflow-target-instance) — CLI `--tm1-instance` > taskfile `settings.tm1_instance` > this key > `default_tm1_instance` (deprecated). | +| `default_tm1_instance` | *(none)* | **Deprecated** alias for `tm1_instance`. Honoured as the final fallback; warns at load only when actually used. | | `default_rushti_cube` | `rushti` | Name of the cube to use | | `dim_workflow` | `rushti_workflow` | Workflow dimension name | | `dim_task_id` | `rushti_task_id` | Task ID dimension name | @@ -206,6 +207,57 @@ All TM1 integration settings live in the `[tm1_integration]` section of `config/ --- +### Per-workflow target instance + +The `tm1_instance` value that receives the results push is resolved against a four-tier precedence chain. The first non-empty value wins: + +| Tier | Source | Where it's set | +|------|--------|----------------| +| 1 | CLI | `rushti run --tm1-instance ` | +| 2 | Taskfile JSON | `settings.tm1_instance` inside the JSON task file | +| 3 | settings.ini (canonical) | `[tm1_integration].tm1_instance` | +| 4 | settings.ini (deprecated) | `[tm1_integration].default_tm1_instance` | + +Tier 2 is the new workflow-level override — drop `tm1_instance` into any JSON task file's `settings` block to push that workflow's results to a specific instance without touching `settings.ini`: + +```json +{ + "version": "2.0", + "metadata": { "workflow": "daily-finance-close" }, + "settings": { + "push_results": true, + "auto_load_results": true, + "tm1_instance": "tm1prod" + }, + "tasks": [ /* ... */ ] +} +``` + +When the upload runs, RushTI logs which tier supplied the value: + +``` +INFO Result upload target: tm1prod (source: taskfile) +``` + +If all four tiers are empty and `push_results` is on, the run still succeeds — RushTI logs a warning and skips the upload: + +``` +WARNING push_results enabled but no TM1 instance configured (CLI --tm1-instance, taskfile settings.tm1_instance, or settings.ini tm1_instance). +``` + +!!! info "Task-level `instance` vs workflow-level `tm1_instance`" + Two `instance`-flavoured fields live at different nesting levels inside a JSON task file: + + - **`tasks[*].instance`** — *task-level execution target.* Where the individual TI process runs. Required on every task. + - **`settings.tm1_instance`** — *workflow-level results target.* Where the run's results are pushed at the end. Optional; overrides settings.ini. + + They are independent. A workflow can execute its tasks across several instances (one per task) and push the consolidated results back to a single target instance. + +!!! warning "Cube-sourced taskfiles have no tier 2" + When a taskfile is loaded from the TM1 cube via `rushti run --tm1-instance X --workflow Y`, the cube schema does not carry a `settings` block — only task definitions. Resolution skips tier 2 and falls through directly to settings.ini (tiers 3 and 4). The CLI value (tier 1) still acts as both the source for the cube read and, in absence of other settings, the fallback results target. + +--- + ## Build Command Options ```bash diff --git a/src/rushti/cli.py b/src/rushti/cli.py index 5389867..fbf80c1 100644 --- a/src/rushti/cli.py +++ b/src/rushti/cli.py @@ -29,7 +29,7 @@ ) from rushti.results_writer import create_results_file from rushti.task import ExecutionMode -from rushti.settings import load_settings, get_effective_settings +from rushti.settings import get_effective_settings, load_settings, resolve_tm1_instance from rushti.taskfile import ( detect_file_type, parse_json_taskfile, @@ -629,42 +629,6 @@ def main() -> int: # Load settings from settings.ini settings = load_settings(cli_args.get("settings_file")) - # Apply CLI overrides to settings - settings = get_effective_settings(settings, cli_args=cli_args) - - if settings.tm1_integration.detailed_results: - logging.info( - "Detailed results enabled: one cube row per executed TI. " - "If you upgraded RushTI from an earlier release, run " - "'rushti build --tm1-instance ' once to add the " - "'original_task_id' measure to the rushti_measure dimension. " - "The build is non-destructive — existing cube data is preserved." - ) - - # Extract final values (CLI overrides settings.ini) - max_workers = ( - cli_args["max_workers"] - if cli_args.get("max_workers") is not None - else settings.defaults.max_workers - ) - process_execution_retries = ( - cli_args["retries"] if cli_args.get("retries") is not None else settings.defaults.retries - ) - result_file = ( - cli_args["output_file"] - if cli_args.get("output_file") is not None - else settings.defaults.result_file - ) - # Resolve result file path relative to application directory (skip if empty/None) - if result_file: - from rushti.utils import resolve_app_path - - result_file = resolve_app_path(result_file) - - logger.debug( - f"Effective settings: max_workers={max_workers}, retries={process_execution_retries}, result_file={result_file}" - ) - # Handle TM1 source if specified tm1_instance = cli_args.get("tm1_instance") workflow = cli_args.get("workflow") @@ -701,6 +665,7 @@ def main() -> int: # Determine workflow and exclusive mode from task file # Parse task file early to get metadata for session context + taskfile_preview = None if tm1_taskfile: # Using TM1 source file_type = "json" # TM1 taskfiles are treated as JSON format @@ -727,6 +692,36 @@ def main() -> int: logger.error(str(e)) sys.exit(1) + # Merge taskfile JSON settings (tier 2) into the effective settings + # before any setting is read. CLI args overlay JSON, which overlays + # settings.ini, which overlays built-in defaults. + effective_taskfile = tm1_taskfile or taskfile_preview + json_settings_dict = effective_taskfile.settings.to_dict() if effective_taskfile else {} + settings = get_effective_settings(settings, json_settings=json_settings_dict, cli_args=cli_args) + + if settings.tm1_integration.detailed_results: + logging.info( + "Detailed results enabled: one cube row per executed TI. " + "If you upgraded RushTI from an earlier release, run " + "'rushti build --tm1-instance ' once to add the " + "'original_task_id' measure to the rushti_measure dimension. " + "The build is non-destructive — existing cube data is preserved." + ) + + # Extract effective execution params (CLI > taskfile JSON > settings.ini > defaults) + max_workers = settings.defaults.max_workers + process_execution_retries = settings.defaults.retries + result_file = settings.defaults.result_file + if result_file: + from rushti.utils import resolve_app_path + + result_file = resolve_app_path(result_file) + + logger.debug( + f"Effective settings: max_workers={max_workers}, " + f"retries={process_execution_retries}, result_file={result_file}" + ) + # Apply exclusive from settings.ini if not set via CLI or JSON if exclusive_mode is None: exclusive_mode = settings.exclusive_mode.enabled @@ -824,16 +819,6 @@ def main() -> int: ) dag.validate() taskfile = tm1_taskfile - # Apply settings from TM1 taskfile (CLI still overrides) - if taskfile.settings.max_workers and cli_args.get("max_workers") is None: - max_workers = taskfile.settings.max_workers - if taskfile.settings.retries is not None and cli_args.get("retries") is None: - process_execution_retries = taskfile.settings.retries - if taskfile.settings.result_file and cli_args.get("output_file") is None: - result_file = resolve_app_path(taskfile.settings.result_file) - logger.debug( - f"Applied TM1 taskfile settings: max_workers={max_workers}, retries={process_execution_retries}" - ) else: # Build DAG from file with dependency validation dag_result = build_dag( @@ -846,16 +831,6 @@ def main() -> int: taskfile = None if isinstance(dag_result, tuple): dag, taskfile = dag_result - # Apply JSON settings (CLI still overrides) - if taskfile.settings.max_workers and cli_args.get("max_workers") is None: - max_workers = taskfile.settings.max_workers - if taskfile.settings.retries is not None and cli_args.get("retries") is None: - process_execution_retries = taskfile.settings.retries - if taskfile.settings.result_file and cli_args.get("output_file") is None: - result_file = resolve_app_path(taskfile.settings.result_file) - logger.debug( - f"Applied JSON settings: max_workers={max_workers}, retries={process_execution_retries}" - ) else: dag = dag_result # TXT source: convert to Taskfile for archiving @@ -1117,12 +1092,15 @@ def main() -> int: assign_unique_task_ids, ) - # CLI --tm1-instance wins over default_tm1_instance for every - # TM1 operation in this run (taskfile read, results push, - # auto_load_results). Silent precedence — no extra warning - # when CLI overrides default. - upload_instance = tm1_instance or settings.tm1_integration.default_tm1_instance + upload_instance, source_tier = resolve_tm1_instance( + tm1_instance, settings, json_settings_dict + ) if upload_instance: + logger.info( + "Result upload target: %s (source: %s)", + upload_instance, + source_tier, + ) tm1_upload = connect_to_tm1_instance( upload_instance, CONFIG, @@ -1173,7 +1151,9 @@ def main() -> int: tm1_upload.logout() else: logger.warning( - "push_results enabled but default_tm1_instance not configured" + "push_results enabled but no TM1 instance configured " + "(CLI --tm1-instance, taskfile settings.tm1_instance, " + "or settings.ini tm1_instance)." ) except Exception as e: logger.warning(f"Failed to upload results to TM1 (non-blocking): {e}") diff --git a/src/rushti/commands/tasks/__init__.py b/src/rushti/commands/tasks/__init__.py index 247e8ac..0d83ec7 100644 --- a/src/rushti/commands/tasks/__init__.py +++ b/src/rushti/commands/tasks/__init__.py @@ -108,10 +108,10 @@ def run_tasks_command(argv: list) -> None: add_taskfile_source_args(push_parser, required=False, include_settings=True) push_parser.add_argument( "--target-tm1-instance", - dest="target_tm1_instance", - default=None, + dest="tm1_instance", + default=argparse.SUPPRESS, metavar="INSTANCE", - help="Target TM1 instance for push (defaults to --tm1-instance if loading from TM1)", + help="[DEPRECATED] Alias for --tm1-instance. Will be removed in a future major version.", ) push_parser.add_argument( "--mode", diff --git a/src/rushti/commands/tasks/push.py b/src/rushti/commands/tasks/push.py index fde259b..0559ae0 100644 --- a/src/rushti/commands/tasks/push.py +++ b/src/rushti/commands/tasks/push.py @@ -1,9 +1,12 @@ """rushti tasks push — upload JSON taskfile to TM1 as a file.""" +import logging import os import sys from pathlib import Path +logger = logging.getLogger(__name__) + def handle_tasks_push(args, config_path: str) -> None: """Handle tasks push action. @@ -11,6 +14,13 @@ def handle_tasks_push(args, config_path: str) -> None: :param args: Parsed arguments :param config_path: Path to config.ini """ + if "--target-tm1-instance" in sys.argv[1:]: + logger.warning( + "DEPRECATION: --target-tm1-instance is deprecated. " + "Use --tm1-instance instead. The deprecated flag will continue " + "to work but may be removed in a future major version." + ) + if not args.taskfile_path: print("Error: --tasks is required for --push (must be a local JSON file)") sys.exit(1) @@ -19,9 +29,9 @@ def handle_tasks_push(args, config_path: str) -> None: print(f"Error: Input file not found: {args.taskfile_path}") sys.exit(1) - target_instance = args.target_tm1_instance or args.tm1_instance + target_instance = args.tm1_instance if not target_instance: - print("Error: --tm1-instance or --target-tm1-instance is required for --push") + print("Error: --tm1-instance is required for --push") sys.exit(1) try: diff --git a/src/rushti/settings.py b/src/rushti/settings.py index ff96344..d44bfdb 100644 --- a/src/rushti/settings.py +++ b/src/rushti/settings.py @@ -69,6 +69,7 @@ class TM1IntegrationSettings: push_results: bool = False auto_load_results: bool = False detailed_results: bool = False + tm1_instance: Optional[str] = None default_tm1_instance: Optional[str] = None default_rushti_cube: str = "rushti" default_workflow_dim: str = "rushti_workflow" @@ -155,6 +156,7 @@ class Settings: "push_results": bool, "auto_load_results": bool, "detailed_results": bool, + "tm1_instance": str, "default_tm1_instance": str, "default_rushti_cube": str, "default_workflow_dim": str, @@ -365,9 +367,67 @@ def load_settings(settings_path: Optional[str] = None) -> Settings: # Resolve paths relative to application directory settings.resume.checkpoint_dir = resolve_app_path(settings.resume.checkpoint_dir) + if not settings.tm1_integration.tm1_instance and settings.tm1_integration.default_tm1_instance: + logger.warning( + "DEPRECATION: [tm1_integration].default_tm1_instance is deprecated. " + "Rename to tm1_instance. The deprecated key will continue to work " + "but may be removed in a future major version." + ) + return settings +def resolve_tm1_instance( + tm1_instance: Optional[str], + settings: Settings, + json_settings: Optional[Dict[str, Any]] = None, +) -> tuple[Optional[str], str]: + """Resolve the effective TM1 instance for results-push, with source tier. + + Walks the 4-tier precedence chain (highest to lowest), returning the + first non-empty value alongside a label identifying the tier: + + 1. CLI argument (``tm1_instance``) → label ``"cli"`` + 2. Taskfile JSON ``settings.tm1_instance`` → label ``"taskfile"`` + 3. ``settings.ini [tm1_integration].tm1_instance`` → label + ``"settings.tm1_instance"`` + 4. ``settings.ini [tm1_integration].default_tm1_instance`` (deprecated) + → label ``"settings.default_tm1_instance"`` + + Empty strings at any tier are treated as unset (fall through). + + ``json_settings`` is the taskfile JSON ``settings`` dict (as returned by + ``TaskfileSettings.to_dict``). It is consulted separately so the + resolver can distinguish tier 2 from tier 3 — by the time + ``get_effective_settings`` has been applied, the JSON-sourced + ``tm1_instance`` has already overwritten the settings.ini value in the + merged ``Settings`` object, and the provenance is otherwise lost. + + :param tm1_instance: Value from the CLI (e.g. ``run --tm1-instance``). + :param settings: Settings object (post-merge). + :param json_settings: Taskfile JSON settings dict; tier 2 lookup. + :return: ``(value, source_label)``; ``(None, "none")`` when all tiers + are empty. + """ + if tm1_instance: + return tm1_instance, "cli" + + if json_settings: + taskfile_value = json_settings.get("tm1_instance") + if taskfile_value: + return taskfile_value, "taskfile" + + settings_value = settings.tm1_integration.tm1_instance + if settings_value: + return settings_value, "settings.tm1_instance" + + deprecated_value = settings.tm1_integration.default_tm1_instance + if deprecated_value: + return deprecated_value, "settings.default_tm1_instance" + + return None, "none" + + def get_effective_settings( settings: Settings, cli_args: Optional[Dict[str, Any]] = None, @@ -419,14 +479,20 @@ def _apply_json_settings(settings: Settings, json_settings: Dict[str, Any]) -> N "push_results": ("tm1_integration", "push_results"), "auto_load_results": ("tm1_integration", "auto_load_results"), "detailed_results": ("tm1_integration", "detailed_results"), + "tm1_instance": ("tm1_integration", "tm1_instance"), } for json_key, (section, attr) in json_to_settings.items(): - if json_key in json_settings and json_settings[json_key] is not None: - section_obj = getattr(settings, section) - value = json_settings[json_key] - setattr(section_obj, attr, value) - logger.debug(f"JSON override: {section}.{attr} = {value}") + if json_key not in json_settings: + continue + value = json_settings[json_key] + if value is None: + continue + if isinstance(value, str) and value == "": + continue + section_obj = getattr(settings, section) + setattr(section_obj, attr, value) + logger.debug(f"JSON override: {section}.{attr} = {value}") def _apply_cli_args(settings: Settings, cli_args: Dict[str, Any]) -> None: diff --git a/src/rushti/taskfile.py b/src/rushti/taskfile.py index 7cf715a..c952f78 100644 --- a/src/rushti/taskfile.py +++ b/src/rushti/taskfile.py @@ -83,6 +83,7 @@ class TaskfileSettings: optimization_algorithm: Optional[str] = None push_results: Optional[bool] = None auto_load_results: Optional[bool] = None + tm1_instance: Optional[str] = None stage_order: Optional[List[str]] = None stage_workers: Optional[Dict[str, int]] = None @@ -103,6 +104,7 @@ def from_dict(cls, data: Dict[str, Any]) -> "TaskfileSettings": optimization_algorithm=data.get("optimization_algorithm"), push_results=data.get("push_results"), auto_load_results=data.get("auto_load_results"), + tm1_instance=data.get("tm1_instance"), stage_order=data.get("stage_order"), stage_workers=data.get("stage_workers"), ) diff --git a/tests/integration/test_settings_integration.py b/tests/integration/test_settings_integration.py index b4754e8..970bcd6 100644 --- a/tests/integration/test_settings_integration.py +++ b/tests/integration/test_settings_integration.py @@ -12,6 +12,7 @@ get_effective_settings, load_settings, resolve_settings_path, + resolve_tm1_instance, ) @@ -261,5 +262,76 @@ def test_load_settings_unknown_key_warning(self): self.assertEqual(settings.defaults.max_workers, 4) +class TestFourTierTm1InstanceResolution(unittest.TestCase): + """End-to-end smoke test for the per-workflow tm1_instance precedence + chain. Walks all four tiers using real files (settings.ini on disk + + taskfile JSON dict) and asserts both value and source label, matching + what the run path does in production.""" + + def setUp(self): + self.temp_dir = tempfile.mkdtemp() + self.config_dir = Path(self.temp_dir) / "config" + self.config_dir.mkdir() + import logging as _logging + + _logging.getLogger("rushti.settings").disabled = False + + def tearDown(self): + import shutil + + shutil.rmtree(self.temp_dir, ignore_errors=True) + + def _write_settings(self, body: str, name: str = "settings.ini") -> str: + p = self.config_dir / name + p.write_text(body) + return str(p) + + def test_four_tier_resolution_end_to_end(self): + """All four precedence tiers, one harness.""" + # Settings.ini has both keys; canonical tier 3, deprecated tier 4. + settings_path = self._write_settings( + "[tm1_integration]\n" + "tm1_instance = ini_canonical\n" + "default_tm1_instance = ini_deprecated\n", + name="settings_both.ini", + ) + + # ---- Tier 4: only the deprecated key is set; canonical is empty. + depr_path = self._write_settings( + "[tm1_integration]\ndefault_tm1_instance = ini_deprecated\n", + name="settings_deprecated_only.ini", + ) + depr_settings = load_settings(depr_path) + value, source = resolve_tm1_instance(None, depr_settings) + self.assertEqual(value, "ini_deprecated") + self.assertEqual(source, "settings.default_tm1_instance") + + # ---- Tier 3: canonical settings.ini key, no taskfile, no CLI. + settings = load_settings(settings_path) + value, source = resolve_tm1_instance(None, settings, json_settings={}) + self.assertEqual(value, "ini_canonical") + self.assertEqual(source, "settings.tm1_instance") + + # ---- Tier 2: taskfile JSON overrides settings.ini. + merged = get_effective_settings( + load_settings(settings_path), + json_settings={"tm1_instance": "taskfile_target"}, + ) + value, source = resolve_tm1_instance( + None, merged, json_settings={"tm1_instance": "taskfile_target"} + ) + self.assertEqual(value, "taskfile_target") + self.assertEqual(source, "taskfile") + + # ---- Tier 1: CLI beats everything. + value, source = resolve_tm1_instance( + "cli_winner", + merged, + json_settings={"tm1_instance": "taskfile_target"}, + ) + self.assertEqual(value, "cli_winner") + self.assertEqual(source, "cli") + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_cli_dispatch.py b/tests/unit/test_cli_dispatch.py index 5a1a7a7..7f24b2d 100644 --- a/tests/unit/test_cli_dispatch.py +++ b/tests/unit/test_cli_dispatch.py @@ -234,42 +234,159 @@ def test_resume_subcommand_returns_context_or_exits(self, monkeypatch): class TestUploadInstancePrecedence: - """Issue #146 regression: --tm1-instance must override default_tm1_instance - for the results push (and the auto_load_results call), not just the - taskfile read.""" - - def test_cli_value_wins_over_default(self): - # The push-results block computes upload_instance as - # `tm1_instance or settings.tm1_integration.default_tm1_instance`. - # When the CLI value is set, it must win. - cli_value = "tm1srv01" - default_value = "CONS_DIM_BUILD" - assert (cli_value or default_value) == "tm1srv01" - - def test_default_used_when_cli_missing(self): - cli_value = None - default_value = "CONS_DIM_BUILD" - assert (cli_value or default_value) == "CONS_DIM_BUILD" - - def test_empty_when_both_missing(self): - cli_value = None - default_value = "" - assert not (cli_value or default_value) - - def test_source_uses_precedence_expression(self): - """Static guard: the push-results path in cli.py must compute the - upload target from the CLI value first. Catches accidental - re-introduction of the shadowing bug where the inner block - reassigned `tm1_instance = settings.tm1_integration.default_tm1_instance` - and silently dropped the CLI value.""" - import inspect - - source = inspect.getsource(cli) - assert "tm1_instance or settings.tm1_integration.default_tm1_instance" in source, ( - "Expected `tm1_instance or settings.tm1_integration.default_tm1_instance` " - "in cli.py — the CLI --tm1-instance must take precedence over the " - "default in the push-results path. See issue #146." + """Issue #146 regression: CLI --tm1-instance must override + default_tm1_instance for the results push (and the auto_load_results + call), not just the taskfile read. + + Originally a literal-string source-code assertion; rewritten as direct + behavioural tests of ``resolve_tm1_instance`` after the 4-tier + resolution refactor.""" + + def test_cli_wins_over_deprecated_default(self): + # Issue #146 regression + from rushti.settings import Settings, resolve_tm1_instance + + settings = Settings() + settings.tm1_integration.default_tm1_instance = "CONS_DIM_BUILD" + value, source = resolve_tm1_instance("tm1srv01", settings) + assert value == "tm1srv01" + assert source == "cli" + + def test_cli_wins_over_canonical_settings(self): + # Issue #146 regression + from rushti.settings import Settings, resolve_tm1_instance + + settings = Settings() + settings.tm1_integration.tm1_instance = "CONS_DIM_BUILD" + value, source = resolve_tm1_instance("tm1srv01", settings) + assert value == "tm1srv01" + assert source == "cli" + + def test_deprecated_default_used_when_cli_missing(self): + # Issue #146 regression + from rushti.settings import Settings, resolve_tm1_instance + + settings = Settings() + settings.tm1_integration.default_tm1_instance = "CONS_DIM_BUILD" + value, source = resolve_tm1_instance(None, settings) + assert value == "CONS_DIM_BUILD" + assert source == "settings.default_tm1_instance" + + def test_none_returned_when_all_tiers_empty(self): + # Issue #146 regression + from rushti.settings import Settings, resolve_tm1_instance + + settings = Settings() + value, source = resolve_tm1_instance(None, settings) + assert value is None + assert source == "none" + + +class TestTasksPushFlagAliasing: + """Spec §12 tests 16–18: --tm1-instance is canonical on `tasks push`; + --target-tm1-instance is a deprecated alias; the alias logs a + deprecation warning.""" + + def test_canonical_flag_works(self, monkeypatch, tmp_path): + from rushti.commands.tasks import run_tasks_command + + taskfile = tmp_path / "t.json" + taskfile.write_text('{"version":"2.0","tasks":[{"id":"1","instance":"tm1","process":"p"}]}') + + called = {} + + def fake_handle_push(args, config_path): + called["tm1_instance"] = args.tm1_instance + + monkeypatch.setattr("rushti.commands.tasks.handle_tasks_push", fake_handle_push) + monkeypatch.setattr( + sys, + "argv", + [ + "rushti", + "tasks", + "push", + "--tasks", + str(taskfile), + "--tm1-instance", + "tm1srv01", + ], ) + run_tasks_command(sys.argv) + assert called["tm1_instance"] == "tm1srv01" + + def test_deprecated_alias_still_works(self, monkeypatch, tmp_path): + from rushti.commands.tasks import run_tasks_command + + taskfile = tmp_path / "t.json" + taskfile.write_text('{"version":"2.0","tasks":[{"id":"1","instance":"tm1","process":"p"}]}') + + called = {} + + def fake_handle_push(args, config_path): + called["tm1_instance"] = args.tm1_instance + + monkeypatch.setattr("rushti.commands.tasks.handle_tasks_push", fake_handle_push) + monkeypatch.setattr( + sys, + "argv", + [ + "rushti", + "tasks", + "push", + "--tasks", + str(taskfile), + "--target-tm1-instance", + "tm1srv01", + ], + ) + run_tasks_command(sys.argv) + assert called["tm1_instance"] == "tm1srv01" + + def test_deprecated_alias_logs_warning(self, monkeypatch, tmp_path, caplog): + import logging + + from rushti.commands.tasks.push import handle_tasks_push + + taskfile = tmp_path / "t.json" + taskfile.write_text("{}") + + class _Args: + taskfile_path = str(taskfile) + tm1_instance = "tm1srv01" + + monkeypatch.setattr(sys, "argv", ["rushti", "tasks", "push", "--target-tm1-instance", "x"]) + # Stub out the actual push so we only exercise the deprecation path. + monkeypatch.setattr( + "rushti.tm1_integration.connect_to_tm1_instance", + lambda *_a, **_kw: (_ for _ in ()).throw(SystemExit(0)), + ) + with caplog.at_level(logging.WARNING, logger="rushti.commands.tasks.push"): + with pytest.raises(SystemExit): + handle_tasks_push(_Args(), config_path="/tmp/cfg.ini") + assert any("--target-tm1-instance is deprecated" in r.message for r in caplog.records) + + def test_canonical_flag_does_not_log_warning(self, monkeypatch, tmp_path, caplog): + import logging + + from rushti.commands.tasks.push import handle_tasks_push + + taskfile = tmp_path / "t.json" + taskfile.write_text("{}") + + class _Args: + taskfile_path = str(taskfile) + tm1_instance = "tm1srv01" + + monkeypatch.setattr(sys, "argv", ["rushti", "tasks", "push", "--tm1-instance", "x"]) + monkeypatch.setattr( + "rushti.tm1_integration.connect_to_tm1_instance", + lambda *_a, **_kw: (_ for _ in ()).throw(SystemExit(0)), + ) + with caplog.at_level(logging.WARNING, logger="rushti.commands.tasks.push"): + with pytest.raises(SystemExit): + handle_tasks_push(_Args(), config_path="/tmp/cfg.ini") + assert not any("--target-tm1-instance is deprecated" in r.message for r in caplog.records) class TestMainBadInputs: diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index dba3a83..334f383 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -13,6 +13,7 @@ get_effective_settings, parse_bool, parse_value, + resolve_tm1_instance, validate_setting, resolve_settings_path, Settings, @@ -273,5 +274,197 @@ def test_resolve_settings_path_returns_new_path_when_neither_exists(self): self.assertFalse(is_legacy) +class TestTm1InstanceResolution(unittest.TestCase): + """Per-workflow tm1_instance resolution: precedence chain CLI > taskfile JSON > + settings.ini tm1_instance > settings.ini default_tm1_instance (deprecated).""" + + def setUp(self): + # logging.config.fileConfig() in cli.py disables existing loggers by + # default when invoked by other tests. Re-enable so assertLogs works. + import logging as _logging + + _logging.getLogger("rushti.settings").disabled = False + + def _write_ini(self, body: str) -> str: + f = tempfile.NamedTemporaryFile(mode="w", suffix=".ini", delete=False) + f.write(body) + f.flush() + f.close() + return f.name + + def test_tm1_instance_in_settings_ini_parses(self): + path = self._write_ini("[tm1_integration]\ntm1_instance = ini_target\n") + try: + settings = load_settings(path) + self.assertEqual(settings.tm1_integration.tm1_instance, "ini_target") + self.assertIsNone(settings.tm1_integration.default_tm1_instance) + finally: + os.unlink(path) + + def test_taskfile_json_overrides_settings_ini(self): + settings = Settings() + settings.tm1_integration.tm1_instance = "from_ini" + result = get_effective_settings(settings, json_settings={"tm1_instance": "from_json"}) + self.assertEqual(result.tm1_integration.tm1_instance, "from_json") + + def test_cli_overrides_taskfile_and_settings_ini(self): + settings = Settings() + settings.tm1_integration.tm1_instance = "from_ini" + json_settings = {"tm1_instance": "from_json"} + value, source = resolve_tm1_instance("from_cli", settings, json_settings) + self.assertEqual(value, "from_cli") + self.assertEqual(source, "cli") + + def test_deprecated_default_alone_is_honoured(self): + path = self._write_ini("[tm1_integration]\ndefault_tm1_instance = legacy_target\n") + try: + with self.assertLogs("rushti.settings", level="WARNING") as cm: + settings = load_settings(path) + self.assertEqual(settings.tm1_integration.default_tm1_instance, "legacy_target") + value, source = resolve_tm1_instance(None, settings) + self.assertEqual(value, "legacy_target") + self.assertEqual(source, "settings.default_tm1_instance") + self.assertTrue( + any("default_tm1_instance" in m and "deprecated" in m.lower() for m in cm.output) + ) + finally: + os.unlink(path) + + def test_canonical_set_alongside_deprecated_does_not_warn(self): + path = self._write_ini( + "[tm1_integration]\n" + "tm1_instance = canon_target\n" + "default_tm1_instance = legacy_target\n" + ) + try: + import logging as _logging + + handler_logs = [] + + class _CaptureHandler(_logging.Handler): + def emit(self, record): + handler_logs.append(self.format(record)) + + target_logger = _logging.getLogger("rushti.settings") + handler = _CaptureHandler(level=_logging.WARNING) + target_logger.addHandler(handler) + try: + settings = load_settings(path) + finally: + target_logger.removeHandler(handler) + + self.assertEqual(settings.tm1_integration.tm1_instance, "canon_target") + self.assertEqual(settings.tm1_integration.default_tm1_instance, "legacy_target") + self.assertFalse( + any("default_tm1_instance" in m for m in handler_logs), + f"Did not expect deprecation warning, got: {handler_logs}", + ) + value, source = resolve_tm1_instance(None, settings) + self.assertEqual(value, "canon_target") + self.assertEqual(source, "settings.tm1_instance") + finally: + os.unlink(path) + + def test_deprecation_warning_fires_exactly_once_at_load(self): + path = self._write_ini("[tm1_integration]\ndefault_tm1_instance = legacy_target\n") + try: + with self.assertLogs("rushti.settings", level="WARNING") as cm: + load_settings(path) + depr_msgs = [m for m in cm.output if "default_tm1_instance" in m] + self.assertEqual(len(depr_msgs), 1, depr_msgs) + finally: + os.unlink(path) + + def test_empty_string_falls_through_to_next_tier(self): + settings = Settings() + settings.tm1_integration.tm1_instance = "from_ini" + # CLI is empty string → treat as unset; should fall through to JSON + json_settings = {"tm1_instance": "from_json"} + value, source = resolve_tm1_instance("", settings, json_settings) + self.assertEqual(value, "from_json") + self.assertEqual(source, "taskfile") + # JSON has empty string → fall through to settings.ini + value, source = resolve_tm1_instance(None, settings, {"tm1_instance": ""}) + self.assertEqual(value, "from_ini") + self.assertEqual(source, "settings.tm1_instance") + + def test_all_empty_returns_none_for_graceful_warning(self): + settings = Settings() + value, source = resolve_tm1_instance(None, settings) + self.assertIsNone(value) + self.assertEqual(source, "none") + + def test_resolve_tm1_instance_returns_label_for_each_tier(self): + # tier 1: CLI + settings = Settings() + v, s = resolve_tm1_instance("cli_val", settings, {"tm1_instance": "json_val"}) + settings.tm1_integration.tm1_instance = "ini_val" + settings.tm1_integration.default_tm1_instance = "depr_val" + self.assertEqual((v, s), ("cli_val", "cli")) + + # tier 2: taskfile JSON + v, s = resolve_tm1_instance(None, Settings(), {"tm1_instance": "json_val"}) + self.assertEqual((v, s), ("json_val", "taskfile")) + + # tier 3: settings.tm1_instance + s_only = Settings() + s_only.tm1_integration.tm1_instance = "ini_val" + v, s = resolve_tm1_instance(None, s_only, json_settings=None) + self.assertEqual((v, s), ("ini_val", "settings.tm1_instance")) + + # tier 4: settings.default_tm1_instance (deprecated) + s_dep = Settings() + s_dep.tm1_integration.default_tm1_instance = "depr_val" + v, s = resolve_tm1_instance(None, s_dep, json_settings=None) + self.assertEqual((v, s), ("depr_val", "settings.default_tm1_instance")) + + +class TestJsonSettingsFlowThrough(unittest.TestCase): + """Spec §12 tests 11–13: taskfile JSON push_results / auto_load_results / + tm1_instance actually take effect via _apply_json_settings.""" + + def test_json_push_results_overrides_settings_ini(self): + settings = Settings() + settings.tm1_integration.push_results = False + result = get_effective_settings(settings, json_settings={"push_results": True}) + self.assertTrue(result.tm1_integration.push_results) + + def test_json_auto_load_results_overrides_settings_ini(self): + settings = Settings() + settings.tm1_integration.auto_load_results = False + result = get_effective_settings(settings, json_settings={"auto_load_results": True}) + self.assertTrue(result.tm1_integration.auto_load_results) + + def test_cli_still_overrides_json_for_tm1_instance(self): + # CLI takes precedence over JSON in the resolver (tier 1 > tier 2) + settings = Settings() + json_settings = {"tm1_instance": "json_val"} + value, source = resolve_tm1_instance("cli_val", settings, json_settings) + self.assertEqual(value, "cli_val") + self.assertEqual(source, "cli") + + +class TestCubeSourceAsymmetry(unittest.TestCase): + """Spec §12 tests 14–15: cube-sourced runs have no tier 2 (taskfile JSON); + settings.tm1_instance (tier 3) still resolves correctly.""" + + def test_cube_source_resolves_to_settings_tm1_instance(self): + """When the cube provides no taskfile-level settings, tier 3 wins.""" + settings = Settings() + settings.tm1_integration.tm1_instance = "cube_target" + # Cube taskfile yields TaskfileSettings() → empty dict + value, source = resolve_tm1_instance(None, settings, json_settings={}) + self.assertEqual(value, "cube_target") + self.assertEqual(source, "settings.tm1_instance") + + def test_cube_source_with_nothing_configured_returns_none(self): + """Cube source with no settings.ini tm1_instance → resolver returns + (None, 'none'); caller logs the graceful warning and skips push.""" + settings = Settings() + value, source = resolve_tm1_instance(None, settings, json_settings={}) + self.assertIsNone(value) + self.assertEqual(source, "none") + + if __name__ == "__main__": unittest.main()