Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes Asimov’s workflow management and packaging by introducing a scheduler abstraction, richer dependency/strategy blueprint features, improved monitoring APIs/state handling, and updates path conventions from C01_offline to analyses.
Changes:
- Switched many pipelines/tests/docs from
C01_offlinetoanalysesand removed hardcodedmasterassumptions. - Added scheduler utilities/abstractions plus programmatic monitor APIs and monitor helper/state components.
- Introduced strategy expansion, prior validation/interfaces (pydantic), additional examples, and expanded docs/tests.
Reviewed changes
Copilot reviewed 121 out of 130 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_pipelines/test_lalinference.py | Update test repo paths to analyses |
| tests/test_pipelines/test_bayeswave.py | Update test repo paths + DAG location assertion |
| tests/test_pesummary_subject_analysis.py | New tests for PESummary as SubjectAnalysis |
| tests/test_optional_dependencies.py | New tests for optional/required dependency handling |
| tests/test_monitor_helpers.py | New unit tests for monitor helper functions |
| tests/test_monitor_api.py | New unit tests for programmatic monitor API |
| tests/test_logging.py | New tests for lazy/rotating file logging behavior |
| tests/test_html_report.py | New tests for HTML rendering enhancements |
| tests/test_data/test_strategy_single.yaml | New test blueprint for single-parameter strategies |
| tests/test_data/test_strategy_matrix.yaml | New test blueprint for matrix strategies |
| tests/test_data/test_strategy_event.yaml | New test event blueprint for strategy tests |
| tests/test_data/blueprints/test_analysis_blueprint.yaml | New basic analysis blueprint for tests |
| tests/test_custom_states.py | New tests for custom monitor states/registration |
| tests/test_cli_manage.py | Update manage CLI tests for analyses path |
| tests/test_blueprints/subject_test_pipeline.yaml | New blueprint for SubjectTestPipeline |
| tests/test_blueprints/simple_test_pipeline.yaml | New blueprint for SimpleTestPipeline |
| tests/test_blueprints/project_test_pipeline.yaml | New blueprint for ProjectTestPipeline |
| tests/test_blueprints/lalinference_quick_test.yaml | New quick-test blueprint for LALInference |
| tests/test_blueprints/gwosc_quick_test.yaml | New quick-test config blueprint |
| tests/test_blueprints/gwosc_get_data.yaml | New blueprint for GW data download |
| tests/test_blueprints/gwosc_event.yaml | New GWOSC event blueprint |
| tests/test_blueprints/bilby_quick_test.yaml | New quick-test blueprint for Bilby |
| tests/test_blueprints/bayeswave_quick_test.yaml | New quick-test blueprint for BayesWave |
| tests/test_blueprints.py | Minimal import test for blueprints module |
| tests/test_asimov.py | Update version detection test to importlib.metadata |
| tests/test_application.py | Add strategy expansion tests |
| tests/test_analysis.py | Improve test isolation by clearing tmp dir |
| tests/mock_gwdatafind_server.py | Add mock gwdatafind HTTP server for tests |
| tests/manual_test_priors.py | Add manual integration script for prior system |
| tests/integration/test_gwtc2d1.py | Update pipeline category argument to analyses |
| tests/integration/GW190426190642.yaml | Update calibration/psd paths to analyses |
| scripts/find_calibration.py | Store calibration assets under analyses |
| scripts/check-ifo.py | Store calibration assets under analyses |
| sandbox/make-nonspin.py | Checkout repository default branch dynamically |
| sandbox/gitlab-issue-tests.py | Update category paths to analyses |
| pyproject.toml | Packaging updates: license, deps, entry points, extra deps |
| examples/strategy_examples.yaml | Add strategy blueprint examples |
| examples/scheduler_example.py | Add scheduler usage example script |
| examples/pesummary_subject_analysis.yaml | Add PESummary SubjectAnalysis example blueprint |
| examples/dependency-examples.yaml | Add dependency syntax examples |
| examples/demo_html_output.py | Demo script for HTML dependency display |
| examples/demo_graph_fix.py | Demo script for graph/dependency modal behavior |
| examples/README.rst | Add examples index documentation |
| examples/README.md | Add dependency management examples documentation |
| examples/MODAL_DEPENDENCIES_GUIDE.md | Visual guide for modal dependency display |
| docs/source/user-guide/running.rst | Update CLI output examples to analyses |
| docs/source/test-interface.rst | Update prior-file example path to analyses |
| docs/source/scheduler-integration.rst | New scheduler integration documentation |
| docs/source/python-api.rst | New Python API documentation |
| docs/source/priors.md | New prior system documentation |
| docs/source/monitor-api.rst | New programmatic monitor API documentation |
| docs/source/index.rst | Add new docs pages to toctrees |
| docs/source/config.rst | Update example ledger paths to analyses |
| docs/source/conf.py | Enable autodoc_pydantic extension |
| docs/source/build-process.rst | Update example ledger paths to analyses |
| docs/source/blueprints.rst | Expand dependency docs + add strategy docs |
| docs/source/api/schedulers.rst | New scheduler API reference page |
| docs/source/api/project.rst | New Project API reference page |
| docs/source/analyses.rst | Document optional deps + SubjectAnalysis notes |
| conda/environment.yaml | Add conda package list for HTCondor tests |
| asimov/strategies.py | New strategy expansion implementation |
| asimov/scheduler_utils.py | New scheduler helper utilities |
| asimov/priors.py | New pydantic-based prior specification system |
| asimov/pipelines/testing/init.py | Add testing pipelines package/export |
| asimov/pipelines/testing/README.md | Document testing pipelines usage |
| asimov/pipelines/rift.py | Switch submit_dag to scheduler API + analyses paths |
| asimov/pipelines/lalinference.py | Add LALInference prior interface + scheduler submit |
| asimov/pipelines/bayeswave.py | Switch submit_dag to scheduler API + logging key change |
| asimov/olivaw.py | Add plugin command discovery + project-aware Click group |
| asimov/monitor_helpers.py | New shared helpers for monitor loop |
| asimov/monitor_context.py | New monitor context object |
| asimov/monitor_api.py | New programmatic monitor API |
| asimov/ledger.py | Use absolute ledger paths + tolerate missing project analyses |
| asimov/git.py | Detect default branch + improve init/push/update behaviors |
| asimov/configs/subjecttestpipeline.ini | Add template for SubjectTestPipeline |
| asimov/configs/simpletestpipeline.ini | Add template for SimpleTestPipeline |
| asimov/configs/projecttestpipeline.ini | Add template for ProjectTestPipeline |
| asimov/configs/lalinference.ini | Use prior interface in config template |
| asimov/configs/bilby.ini | Use prior interface + sampler/additional files hooks |
| asimov/configs/README.rst | Document scheduler configuration usage |
| asimov/condor.py | Use scheduler abstraction internally + htcondor2 support |
| asimov/cli/project.py | Move logging config to logging.location + lazy log setup |
| asimov/cli/manage.py | Ensure file logging setup + SubjectAnalysis readiness gating |
| asimov/cli/blueprint.py | Add blueprint validation CLI |
| asimov/asimov.conf | Default calibration_directory to analyses |
| asimov/init.py | Replace pkg_resources, add lazy rotating file logging |
| LICENSE | Update license text/formatting |
| IMPLEMENTATION.md | Document dependency system implementation details |
| CHANGELOG.rst | Add 0.7.0-alpha2 changelog entry |
| .gitlab-ci.yml | Add HTCondor container-based CI job |
| .github/workflows/testing-pipelines.yml | Add HTCondor-based pipeline integration workflow |
| .github/workflows/python-app.yml | Expand python matrix + add publish/release automation |
| .github/workflows/docs.yml | Add docs build + GitHub Pages deployment |
| .github/actions/wait-for-files/action.yml | Add composite action to wait for output artifacts |
| .github/actions/setup-htcondor/action.yml | Add composite action to start HTCondor |
| .github/actions/setup-asimov-env/action.yml | Add composite action to install deps with conda |
| .github/actions/run-asimov-command/action.yml | Add composite action to run Asimov as submituser |
| .github/actions/create-submit-user/action.yml | Add composite action to create submituser |
Comments suppressed due to low confidence (3)
tests/test_pipelines/test_bayeswave.py:43
- This test previously staged
Prod1_test.inibut now stagesProd0_test.ini. Later in this test file the DAG path references.../Prod1/..., so the staged ini name looks inconsistent and may cause the pipeline setup to use the wrong config. Stage the ini file that matches the production being exercised (likelyanalyses/Prod1_test.ini).
os.system("git add analyses/Prod0_test.ini analyses/s000000xx_gpsTime.txt")
asimov/git.py:90
- Inside the exception handler, logging references
self.event, which may not exist on this class (or may be unset), potentially raising anotherAttributeErrorand preventing the intended fallback. Use a safe identifier (e.g.,getattr(self, 'event', None)orself.directory) in the warning message.
except (git.exc.GitCommandError, AttributeError) as e:
# In case of any error, return 'master' as a safe default
self.logger.warning(f"Could not detect default branch for {self.event}: {e}")
return 'master'
docs/source/blueprints.rst:359
- This section uses
marginalisation(British spelling), but other examples in this PR (e.g.,examples/strategy_examples.yaml) usemarginalization. This inconsistency can confuse users and may not match the actual config keys. Align the spelling to the canonical key used by the project/templates.
Strategy parameters can use dot notation to set deeply nested values::
kind: analysis
name: bilby-margdist-{likelihood.marginalisation.distance}
pipeline: bilby
strategy:
likelihood.marginalisation.distance:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| click.echo("Try running `asimov manage build` first.") | ||
| try: | ||
| pipe.submit_dag(dryrun=dryrun) | ||
| cluster_id = pipe.submit_dag(dryrun=dryrun) |
There was a problem hiding this comment.
submit_dag() returns inconsistent shapes across pipelines (e.g., BayesWave returns (cluster_id,), others may return (cluster_id, logger)), so int(cluster_id) can raise TypeError. Normalize cluster_id here the same way you already do later in the file (extract first element for tuple/list) before casting/assigning.
| cluster_id = pipe.submit_dag(dryrun=dryrun) | |
| cluster_id = pipe.submit_dag(dryrun=dryrun) | |
| # Normalise cluster_id in case submit_dag returns a tuple/list, | |
| # e.g., (cluster_id,) or (cluster_id, logger). | |
| if isinstance(cluster_id, (tuple, list)): | |
| cluster_id = cluster_id[0] |
| # Only set up file handler once (thread-safe check) | ||
| with _file_handler_lock: | ||
| if _file_handler is not None: | ||
| return | ||
|
|
||
| # Determine log file location |
There was a problem hiding this comment.
The lock only protects the initial _file_handler is not None check; the handler creation/assignment happens outside the lock, so concurrent calls can still create and attach multiple handlers. Keep the entire setup (including determining logfile, creating RotatingFileHandler, and setting _file_handler) within the locked section, or re-check _file_handler immediately before assignment inside the lock.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
asimov/cli/application.py:180
- The KeyError message interpolates
{event}, which is the CLI argument and may be None; the lookup is done withevent_s. Useevent_sin the message so the user sees the actual event name that couldn't be found.
click.style("●", fg="red")
+ f" Could not apply a production, couldn't find the event {event}"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ledger.data["history"][event_obj.name] = history | ||
| ledger.save() | ||
| update(ledger.events[event.name], event.meta) | ||
| ledger.events[event.name]["productions"] = analyses | ||
| ledger.events[event.name].pop("ledger", None) | ||
| update(ledger.events[event_obj.name], event_obj.meta) | ||
| ledger.events[event_obj.name]["productions"] = analyses | ||
| ledger.events[event_obj.name].pop("ledger", None) |
There was a problem hiding this comment.
In the update_page=True + event_exists branch, ledger.save() is called before mutating ledger.events[event_obj.name] (meta update, productions assignment, and popping ledger). Those mutations won't be persisted to disk unless you save again (or use ledger.update_event(...), which saves). Move the save after the mutations or add a second ledger.save() once the event dict has been updated.
Description
Brief description of what this PR does.
Type of Change
Motivation and Context
Why is this change needed? What problem does it solve?
Fixes #(issue number) (if applicable)
Changes Made
List the main changes:
Testing
How has this been tested?
Test configuration:
Checklist
Documentation
Breaking Changes
Does this PR introduce any breaking changes? If yes, describe:
Additional Notes
Any additional information, concerns, or considerations for reviewers.
Screenshots (if applicable)
For UI or output changes, include before/after screenshots or terminal output.
Reviewer Notes: