Refactor introduce runners - #205
Open
mschwoer wants to merge 19 commits into
Open
Conversation
--- # Conversation that produced these changes --- ## User prompt "A new job handler will be added soon, which will execute a command via SSH. This can target both windows and linux machines. Those can have backup,output,settings paths mapped differently (linux: mount, windows: drive letter or UNC). Help me design an upfront refactoring. Goal: one deployment runs jobs on several named runners, including hosts whose OS and path flavour differ from the Airflow worker's. Prepares the seam for an SSH job handler. Keep it as simple as possible." ## Clarifying round 1 Q: How should `Settings.job_engine` become a runner reference in the DB? - Rename to `runner`, with migration (Recommended) <-- chosen - Keep DB field `job_engine`, reinterpret as runner name - Rename to `runner` via mongoengine `db_field` Q: `_check_content` rejects every `\` and `:`, so no Windows path can be exported today. Include the validation change in this refactoring? - Yes, validate relative parts only (Recommended) <-- chosen - No, leave validation for the SSH handler work Q: The docker runner today exports cluster-view paths and binds host paths onto them. Keep that, or make the docker runner export docker-host paths? - Keep: docker runner uses the default view (Recommended) <-- chosen - Change: export host paths, bind same path Q: When a yaml has no `runners:` block, what should happen? - Fail at import with a clear error (Recommended) <-- chosen - Implicit default runners `slurm`, `docker`, `file_based` Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "so if we introduced a absolute_backup_base_path (maybe next to mounts_path), we could move all absolute_paths to the runners?" "yes, update the spec and commit with conversation" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt ""locations" now is mount-specific, right? move backup, etc under a "mounts" level, keep only "general" as is" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "make runners a list, move the current key (e.g. "win_box") to a "name" attribute" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "no defaults for os and ssh_connection_id_prefix" ""rejected" ? or "ignored"?" "ok, ignored, update the spec and commit with conversation" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… in the spec --- # Conversation that produced these changes --- ## User prompt "add to the spec why conncetions should stay in airflow (this should be a comment or in the docs). Also, document the reason for ssh_connection_id_prefix instead of list" "locations.slurm: is there any validation planned ?" "yes, add it, update the spec and commit with conversation" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "runners[i].locations clashes with the toplevel key .. suggestions?" "I prefer having backup_absolute_path with the other paths .. go with 1.1" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…op-level, MOUNTS_PATH from env --- # Conversation that produced these changes --- ## User prompt "I made changes to the SPEC.md, adapt if needed" "where is mounts_base_path used?" "but MOUNTS_PATH is in the environmental variables? why can't it be read?" ""the Docker daemon rejects relative bind sources", why would this be the case?" "this is a bout the msqc container?" "could the docker runner be made compatible with relative mounts_paths ?" "nah, we just add a comment that MOUNTS_PATH needs to be absolute in case the docker runner is used" "move backup_base_path to the backup toplevel key. then move mounts one level down" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "os: macos should be allowed for completeness, treated like "linux"" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "/ms-review-spec Review @SPEC.md as a skeptical implementer about to start coding tomorrow. For each finding, quote the offending line or section (using "|" as prefix) and propose a concrete fix. Group as: - Must-fix — would cause incorrect code, contradicts another part of the spec, or leaves a decision the implementer can't make alone. - Defer — worth tracking but doesn't block a correct first implementation. Focus on unclarities, overengineering, logic holes, obvious bugs. Be succint and add your findings to the bottom of SPEC.md in a section called "REVIEW"." ## User prompt "/ms-apply-spec-review I added feedback from a review to the bottom of @SPEC.md, together with my diretions on them in lines starting with "USER_COMMENT". Please update the SPEC.md based on this, and add a short one-liner describing your change (starting with "AGENT_RESPONSE") after each USER_COMMENT." ## User prompt "apply d10 only" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "/agent-skills:plan create a plan for SPEC.md. Aim for self-contained work packages that each leaves a working state." Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "/agent-skills:plan create a plan for SPEC.md. Aim for self-contained work packages that each leaves a working state." ## User prompt "compare to the plan in @tasks/" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
--- # Conversation that produced these changes --- ## User prompt "compare your plan against @tasks/alternative_plan.md" "rewrite tasks/plan.md and tasks/todo.md" Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Task 1 of tasks/todo.md. The yaml key locations.general.mounts_path stays, unread, until T10. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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
1 participant
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.
Add spec for preparing for arbitrary runners and first slice.