Make JWD a job concern#23133
Conversation
github-actions
Bot
added
area/testing
area/API
area/jobs
area/database
0d7e4c3 to
f172198
Compare
| # set_job_destination() assigns job.destination_params = new_destination.params | ||
| # and flushes, so when clear_working_directory() → _setup_working_directory() | ||
| # → _set_working_directory() reads self.job_destination.params, which now | ||
| # holds the *new* params, not the stale values from the prior attempt. |
There was a problem hiding this comment.
A comment here is good - maybe just the first line and last line though - these four lines in the middle seem like noise to me.
Really nice catch on the location of delay bug - I think this is a very long existing bug with existing issues?
There was a problem hiding this comment.
Thanks, I've reduced that comment. Of course it wasn't me who detected the bug 😅
| # _setup_working_directory() → _set_working_directory() mutates the | ||
| # working_directory column, so the column survives the sa_session.refresh() | ||
| # that mark_as_resubmitted() performs at the end of this flow even when no | ||
| # handler commit runs below. |
There was a problem hiding this comment.
Just drop this second comment? You're literally restating abstraction details the abstraction is meant to hide I think? Feel free to correct me if I am wrong.
There was a problem hiding this comment.
You are wrong in very rare cases, and this is not one of them 😆
For some reason, my agent was obsessed with writing archaeology comments all over this code, and I was not familiar enough with it to decide what was important to keep and what wasn't. So thank you again.
| # is resolved, so job.working_directory is None. This site uses | ||
| # extra_dir=str(job.id), a different path scheme than the obj_dir=True sites | ||
| # that JobWorkingDirectory manages. Keep the direct object-store call so the | ||
| # path matches what later metadata file lookups expect. |
There was a problem hiding this comment.
These are accidentally different instead of purposefully different though right? Can you have your agent research if these could be migrated to use the new abstraction and if not if we could do a parallel abstraction in the same job_execution/ file for this - so we're keeping like-callers and like concerns close together? This can be a follow up as long as there is an issue I think.
There was a problem hiding this comment.
There was a bit of back and forth in the commits about this (and the next comment). I think it was the test failures that triggered the change, and again, I was not comfortable enough arguing with my agent, who was very sure this was the correct approach 😅 Turns out I asked again, and now it thinks it is perfectly fine to use the same abstraction and forget about the extra_dir thing... here is what it says now:
The earlier test failures that drove the revert were likely caused by the extra_dir/legacy_extra_dir parameter being passed in addition to obj_dir=True inside JobWorkingDirectory (see the a34f6d2 version of setup.py — it passed both obj_dir=True AND extra_dir=extra_dir to the object store). That combination produces /000/001/<job.id>/<job.id> — a doubled path — which is genuinely different and would break lookups. My migration drops extra_dir entirely and relies on obj_dir=True alone, which matches the original path exactly.
🤷♂️
|
While playing with the TPV pull request I'm working on, I noticed that the I'll fix that... |
|
Now it works in combination with galaxyproject/total-perspective-vortex#202 With a runners:
local:
load: galaxy.jobs.runners.local:LocalJobRunner
workers: 4
execution:
default: tpv_dispatcher
environments:
tpv_dispatcher:
runner: dynamic_tpv
tpv_config_files:
- /<your galaxy root>/config/tpv_mapping.ymlAnd this global:
default_inherits: default
context:
jwd_pool:
- value: /<some path>/jobs_directory/fast
weight: 3
- value: /<some path>/jobs_directory/slow
weight: 1
tools:
default:
cores: 1
mem: 2
params:
job_working_directory: "{helpers.weighted_choice(jwd_pool)}"
scheduling:
require: []
prefer:
- general
accept: []
reject: []
destinations:
local:
runner: local
max_accepted_cores: 4
max_accepted_mem: 8
scheduling:
prefer:
- generalThe JWD of each job will look something like |
Introduce a new column to store the working directory path for jobs within the database. This allows for better tracking of job execution environments and facilitates easier debugging or retrieval of job-related files.
- Eliminates construction-order sensitivity by reading the directory column fresh on each access - Clarifies parameter naming to restrict legacy object-store subdirectory handling - Establishes a canonical path validator enforced at both assignment and usage times
- Replaces inline path checks with the centralized validation function - Defers session persistence to callers to improve transaction management flexibility - Ensures working directory mutations survive database refresh operations during job resubmission
- Adjusts documentation to reflect deferred session persistence behavior - Migrates existing callers to use the renamed legacy parameter - Refactors integration tests to consistently utilize the working directory abstraction
Adds an existence check before attempting to remove the custom working directory path. Prevents exceptions if the directory was already cleaned up or never created.
Refactors metadata actions and job files API to bypass the standard working directory class. Direct object store calls maintain compatibility with a specific legacy path scheme required by the site. Removes unused imports.
Working directory configuration now triggers during initialization and references the params attribute. Initializing it beforehand prevents an AttributeError when resolving the job destination.
The previous implementation relied on a configuration fallback that incorrectly mapped the global object-store base directory to individual jobs. Reading directly from the destination parameters prevents this unintended override and ensures resubmitted jobs correctly resolve their custom working directories.
The extra directory argument was only relevant for historical object store paths and introduced unnecessary complexity. Dropping it streamlines the API and eliminates outdated error checks.
Reads destination parameters directly from the persisted job object to prevent incorrectly storing the default configuration value. Bypasses the mapper's cached destination to resolve stale data issues during resubmit and reorder operations. Removes outdated comments regarding legacy fallback behavior.
Removes excessive implementation details regarding session flushing and commit paths that cluttered the workflow.
Replaces direct object-store calls with a centralized approach for creating and resolving job working directories.
Additionally, directory creation now fails loudly if a path already exists, preventing silent overwrites from ID collisions or stale runs.
Streamlines documentation by removing verbose implementation details and historical design rationales.
Prevents path collisions and ensures consistent directory sharding when custom working directories are configured. Validates resolution, creation, cleanup, and cleared contents base logic while confirming the default object-store behavior remains intact.
Replaces direct object store and filesystem mocking with a dedicated test fixture. This simplifies setup, removes unnecessary temporary directory manipulations, and shifts focus to the core logic and error handling.
This improves code readability and ensures paths are explicitly validated before use.
cd8c2e9 to
bb4740d
Compare
2 participants
Implements Phase A of the plan discussed in #23107: decouple the job working directory (JWD) from the object store and make it a job-level concern, resolved once from destination params and persisted on the
Jobrow. This closes the proposed use case from #15616 (overridingjob_working_directoryper destination via TPV) and lays the groundwork for the weighted-pool / drain behavior requested in #20062.The approach follows the counter-proposal in the discussion: mirror how
object_store_idalready flows (read from destination params inenqueue(), persisted on the row) rather than hash-deriving from a weighted pool. No config schema, no resolver service, no weighting code in Galaxy.Related TPV pull request galaxyproject/total-perspective-vortex#202
What changed
New
Job.working_directorycolumn (String(1024), mirroring the existingTask.working_directory) added via migratione96dd6fd5863. Persisted when a destination specifiesjob_working_directory;NULLmeans "use the legacy object-store path".JobWorkingDirectoryclass inlib/galaxy/job_execution/setup.pycentralizes all JWD operations (resolve,exists,create,delete,cleared_contents_base) behind a single handle, replacing the open-codedobject_store.get_filename(job, base_dir="job_work", dir_only=True, obj_dir=True)calls scattered across the codebase. It transparently picks the custom-path strategy whenjob.working_directoryis set and falls back to the object-store strategy otherwise, so callers never branch on the column.The custom path is treated as a parent/base: Galaxy appends
<directory_hash_id(job.id)>/<job.id>/for per-job isolation, mirroring the object-store layout. This means ajob_working_directorydestination param (e.g. set by TPV from a weighted pool, see Addweighted_choicehelper for weighted random selection in TPV configs total-perspective-vortex#202) is the base directory, and each job gets its own subdirectory under it._set_working_directoryinMinimalJobWrapperresolvesjob_working_directoryfrom the persistedjob.destination_params(not viaget_destination_configuration, which would fall back to the config default and persist it even for destinations that don't specify one) and writes it tojob.working_directory. Called from_setup_working_directorybefore anything is created on disk. Reset toNoneon resubmit when the new destination no longer specifies a custom path, so stale values don't survive.validate_working_directory_pathguards every disk operation on a custom path (and at set-time): refuses empty, relative, or filesystem-root paths, sincejob.working_directoryis an admin/TPV-supplied string thatshutil.rmtreeobeys literally.Resubmit reorder in
resubmit.py:set_job_destination(which persists the newdestination_params) now runs beforeclear_working_directory, so the latter's_setup_working_directory→_set_working_directoryreads the new params and resolves the new JWD correctly. Previously the order was inverted, so a resubmitted job resolved its JWD against the stale destination.clear_working_directorynow flushes so the mutatedworking_directorycolumn survives thesa_session.refresh()thatmark_as_resubmitted()performs.Init-order fix in
MinimalJobWrapper.__init__:self.paramsis now initialized before_setup_working_directory, since that call chain reachesget_destination_configurationvia thejob_destinationproperty.Callers migrated to
JobWorkingDirectory:MinimalJobWrapper(working_directory,working_directory_exists,_create_working_directory,clear_working_directory,finishcleanup),JobManagerstdout/stderr tail,SetMetadataTool.exec_after_process,ExportHistoryToolAction, the Celerycleanup_jwdstask,tools/actions/metadata.py(out-of-band metadata job), andwebapps/galaxy/api/job_files.py(Pulsar staging security check). The previously out-of-band sites that usedextra_dir=str(job.id)are now also routed throughJobWorkingDirectory, which resolves to the same path viaobj_dir=True.Why
weighted_choicehelper for weighted random selection in TPV configs total-perspective-vortex#202) spread jobs across severaljob_working_directorymounts, enabling load balancing across disks or nodes. Because each job's JWD is resolved once and stored on the row, in-flight jobs are unaffected when the distribution rule changes.job.destination_params, and recovery rebuilds fromfrom_job=jobwithout re-running the rule.working_directoryno longer raisesObjectNotFoundfor jobs whose object store hasn't been assigned yet.Tests
test/unit/job_execution/test_job_working_directory.pycoveringJobWorkingDirectoryper-job isolation of custom paths (resolve, create, delete, cleared_contents_base, validation).test/unit/app/test_tasks.py(cleanup task),test/integration/test_job_files.py, andtest/integration/test_pulsar_embedded.pyto use theJobWorkingDirectoryabstraction.How to test the changes?
See Make JWD a job concern #23133 (comment) for configuration examples.
License