Fix submit logic adding for LensingFlow production analyses#104
Fix submit logic adding for LensingFlow production analyses#104disharh wants to merge 26 commits intoetive-io:v0.6-releasefrom
Conversation
Make v0.5.9 release See merge request asimov/asimov!165
DOC Expand the pipelines documentation See merge request asimov/asimov!155
DOCS Analysis remixing See merge request asimov/asimov!169
Fix documentation TOC See merge request asimov/asimov!171
- Introduced a bug report template for users to report issues effectively. - Added a feature request template to gather user suggestions for enhancements. - Created a pull request template to standardize contributions and improve review processes. - Updated the README with comprehensive information about Asimov's features and usage. - Added a CONTRIBUTORS file to acknowledge contributions from the community. - Included a USERS file to showcase research groups and institutions utilizing Asimov. - Established a CITATION file for proper attribution in academic work.
|
NB this MR was made from the main branch, but is a bugfix specific to the 0.6 releases, so I've chnaged the target branch. This means that some additional files are now included in the MR which we'll need to tidy-up before merge. |
There was a problem hiding this comment.
Pull request overview
This PR primarily removes custom submission/priority logic previously added for handling LensingFlow-related productions, reverting asimov manage submit behavior closer to upstream defaults. It also introduces a substantial documentation + repository metadata refresh (new tutorials, README rewrite, community files, and GitHub templates).
Changes:
- Reverts/removes specialized submission gating/priority logic in
asimov/cli/manage.pyfor productions and project analyses. - Fixes an indentation issue in
asimov/cli/review.pyand updates a Bilby config template to skipNone-valued prior parameters. - Adds/updates multiple docs pages and repo meta files (tutorials, README, CITATION/USERS/CONTRIBUTORS, GitHub templates).
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
asimov/cli/manage.py |
Removes specialized submit logic and refactors project-analysis “interest” handling and priority adjustment. |
asimov/cli/review.py |
Fixes indentation in project-analysis review add flow. |
asimov/configs/bilby.ini |
Avoids emitting None values when rendering luminosity distance prior kwargs. |
docs/source/index.rst |
Reorganizes docs landing page and adds Tutorials section to navigation. |
docs/source/getting-started.rst |
Adds explicit Getting Started ref label and whitespace tidy-ups. |
docs/source/citing.rst |
Fixes link formatting/spacing in citation guide. |
docs/source/tutorials/analysing-gw150914.rst |
Adds a new end-to-end tutorial for GW150914. |
docs/source/tutorials/adding-a-pipeline.rst |
Adds a new tutorial on integrating a pipeline. |
docs/source/tutorials/remixing-analyses.rst |
Adds a new tutorial on remixing published analyses. |
README.md |
Major rewrite/expansion of README content and structure. |
USERS.md |
Adds a users/adopters page for the project. |
CONTRIBUTORS.md |
Adds a contributors listing and contribution guidance. |
CITATION.cff |
Adds a Citation File Format metadata file. |
.github/pull_request_template.md |
Adds a GitHub PR template. |
.github/ISSUE_TEMPLATE/bug_report.md |
Adds a GitHub bug report issue template. |
.github/ISSUE_TEMPLATE/feature_request.md |
Adds a GitHub feature request issue template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # check if we need to account for extra priority comming from atlenstics | ||
| if "extra priority" in analysis.meta["needs settings"].keys(): | ||
| extra_prio_pipeline = analysis.meta["needs settings"]["extra priority"] | ||
| if extra_prio_pipeline in interest_dict_project_analyses.keys(): | ||
| if subj_string in interest_dict_project_analyses[extra_prio_pipeline].keys(): | ||
| extra_prio = interest_dict_project_analyses[extra_prio_pipeline][subj_string]["interest status"] | ||
|
|
||
| if ( | ||
| analysis.meta["needs settings"]["extra priority"] | ||
| == "atlenstics_compatibility" | ||
| ): |
There was a problem hiding this comment.
This extra priority block indexes analysis.meta['needs settings'] without first checking that needs settings exists on the analysis. If a project analysis has dependencies but no needs settings, this will raise and prevent submissions. Add a guard (or use .get) around analysis.meta['needs settings'] before accessing .keys() / fields.
| if interest_dict[old_analysis][-1]["interest status"] is True: | ||
| interested_pipelines += 1 |
There was a problem hiding this comment.
The running-job priority recalculation uses interest_dict[old_analysis][-1] and ignores the stored subjects field. If multiple subject-sets exist per pipeline, this can apply interest from a different subject group and adjust the wrong job’s priority. Filter interest_dict[old_analysis] by matching subjects (as done earlier in the submission gating logic).
| if interest_dict[old_analysis][-1]["interest status"] is True: | |
| interested_pipelines += 1 | |
| # Filter interest entries by matching subjects, to avoid | |
| # applying interest from a different subject group. | |
| current_subjects = analysis.meta.get("subjects") | |
| matching_entries = [ | |
| entry | |
| for entry in interest_dict[old_analysis] | |
| if entry.get("subjects") == current_subjects | |
| ] if current_subjects is not None else [] | |
| if matching_entries: | |
| if matching_entries[-1].get("interest status") is True: | |
| interested_pipelines += 1 | |
| else: | |
| # Fallback to previous behaviour if no subjects match | |
| # (or if subjects are not recorded), to avoid changing | |
| # semantics when subject information is unavailable. | |
| if interest_dict[old_analysis][-1].get("interest status") is True: | |
| interested_pipelines += 1 |
| if interested_pipelines < 2: | ||
| theoretical_prio = 0 | ||
| else: | ||
| theoretical_prio = int((interested_pipelines-analysis.meta["needs settings"]["minimum"])*10) | ||
| theoretical_prio = int((interested_pipelines - 2) * 10) |
There was a problem hiding this comment.
This priority formula hard-codes a threshold of 2 interested pipelines, rather than using the configured meta['needs settings']['minimum'] value (which is used earlier in the function). This makes the runtime priority behavior inconsistent with the submission gating settings and may mis-prioritize jobs when the minimum differs from 2.
| .. toctree:: | ||
| :maxdepth: 1 | ||
| :Caption: Tutorials | ||
|
|
There was a problem hiding this comment.
The toctree option is written as :Caption: but Sphinx/reST expects :caption: (option names are case-sensitive). As-is, this will likely be ignored and may emit a docs build warning/error.
|
|
||
| .. code-block :: console | ||
|
|
||
| $ asimov apply -f get-data.yaml -e GW150914_095045 |
There was a problem hiding this comment.
This command applies get-data.yaml again, but at this point the tutorial says to apply bilby-IMRPhenomPv2.yaml. As written, readers will never add the bilby remix analysis to the project.
| $ asimov apply -f get-data.yaml -e GW150914_095045 | |
| $ asimov apply -f bilby-IMRPhenomPv2.yaml -e GW150914_095045 |
| interest_dict_project_analyses = {} | ||
| # this should add the required repository field to the project analysis | ||
| # with the correct type | ||
| # FIXME: for now, this only accountd for the case where we have multiple analysis |
There was a problem hiding this comment.
Typo in comment: “only accountd” should be “only accounts”.
| # FIXME: for now, this only accountd for the case where we have multiple analysis | |
| # FIXME: for now, this only accounts for the case where we have multiple analysis |
| @@ -0,0 +1,116 @@ | |||
| .. Remixing analyses_ | |||
There was a problem hiding this comment.
This looks like it was intended to be a reST reference label, but .. Remixing analyses_ is not a valid label directive. If you want this page to be linkable via :ref:, use the label form .. _remixing-analyses: (or similar).
| .. Remixing analyses_ | |
| .. _remixing-analyses: |
| name: bilby-IMRPhenomPv2 | ||
| pipeline: bilby | ||
| waveform: | ||
| approximant: IMRPhenomPv2 |
There was a problem hiding this comment.
The YAML example is invalid as written: approximant is not indented under waveform:. Readers copying this will get a YAML parse error or a mis-specified waveform block. Indent approximant as a child key of waveform.
| approximant: IMRPhenomPv2 | |
| approximant: IMRPhenomPv2 |
| import asimov.pipeline.Pipeline | ||
|
|
||
| class pyRing(asimov.pipeline.Pipeline): |
There was a problem hiding this comment.
The Python example import is not valid: import asimov.pipeline.Pipeline won’t import the Pipeline symbol, and asimov isn’t imported for asimov.pipeline.Pipeline usage in the class definition. Update the snippet to a valid import pattern so readers can run it as-is.
| import asimov.pipeline.Pipeline | |
| class pyRing(asimov.pipeline.Pipeline): | |
| from asimov.pipeline import Pipeline | |
| class pyRing(Pipeline): |
| ## Table of Contents | ||
|
|
||
| ### Job monitoring and management | ||
| - [What is Asimov?](#what-is-asimov) | ||
| - [Why Asimov?](#why-asimov) | ||
| - [Key Features](#-key-features) | ||
| - [Primary Use Case: Gravitational Waves](#-primary-use-case-gravitational-waves) | ||
| - [Beyond Gravitational Waves](#-beyond-gravitational-waves) | ||
| - [Installing Asimov](#-installing-asimov) | ||
| - [Quick Start](#-quick-start) | ||
| - [Do I Need Asimov?](#-do-i-need-asimov) | ||
| - [Example Workflow](#-example-workflow) | ||
| - [Extensibility](#-extensibility) | ||
| - [Roadmap](#-roadmap) | ||
| - [Contributing](#-contributing) | ||
| - [Citation](#-citation) | ||
| - [Authors](#-authors) |
There was a problem hiding this comment.
The PR description focuses on reverting submit logic and fixing an indentation error, but this PR also introduces substantial new documentation/tutorial pages and repository meta files (README rewrite, new templates, CITATION/USERS/CONTRIBUTORS). Consider updating the PR description to reflect these additional changes or splitting them into separate PRs to keep review scope focused.
Description
Remove some project analyses related submit logic added for handling LensingFlow analyses.
Type of Change
Motivation and Context
The production analyses submit logic which looks for the production number in analysis name and decides submission priority, has been removed. This was added for the purpose of handling LensingFlow project analyses but LensingFlow now incorporates handling multiple productions internally.
Changes Made
List the main changes:
.subjectscorrected to._subjects)Testing
How has this been tested?
Test configuration:
/home/disha.hegde/.conda/envs/lfcloneChecklist
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: