[ENH] Allow unrecognized pipeline names and versions in processing status file #401

alyssadai · 2024-12-03T19:34:09Z

Closes Emit warning instead of erroring out when pipeline/version not recognized #382

Changes proposed in this pull request:

Checks of pipeline names and versions updated so that:
- If no available pipeline names are recognized, error out, otherwise emit a warning and continue
- For each recognized pipeline, emit a warning if >=1 versions are unrecognized and continue
  - If no versions of any recognized pipelines are themselves recognized, error out

For reviewer: Sourcery was super verbose on this PR because it seemingly reviewed it twice (I accidentally marked the PR ready for review before I was ready, and then once I converted to draft and then back to ready for review, the old comments stuck around even though several were no longer applicable). Not sure how to make Sourcery auto-resolve old reviews - I attempted to do it using a command but that only triggered yet another review 😬 To make this PR easier to read, I've left only the most recent set of comments unresolved.

Checklist

This section is for the PR reviewer

PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
PR has a label for the release changelog or skip-release (to be applied by maintainers only)
PR links to GitHub issue with mention Closes #XXXX
Tests pass
Checks pass

For new features:

Tests have been added

For bug fixes:

There is at least one test that would fail under the original bug conditions.

Summary by Sourcery

Enhance the processing status file handling by allowing unrecognized pipeline names and versions, emitting warnings instead of errors when some are unrecognized, and ensuring at least one recognized pipeline-version combination is present. Update tests to cover these scenarios.

Enhancements:

Update pipeline name and version checks to allow unrecognized entries, emitting warnings instead of errors when some are unrecognized.

Tests:

Add tests to ensure unrecognized pipelines and versions are excluded from output with warnings, and that errors are raised when no recognized pipeline-version combinations exist.

Summary by Sourcery

Enhance the processing status file handling by allowing unrecognized pipeline names and versions, emitting warnings instead of errors when some are unrecognized, and ensuring at least one recognized pipeline-version combination is present. Update tests to cover these scenarios.

Enhancements:

Update pipeline name and version checks to allow unrecognized entries, emitting warnings instead of errors when some are unrecognized.

Tests:

Add tests to ensure unrecognized pipelines and versions are excluded from output with warnings, and that errors are raised when no recognized pipeline-version combinations exist.

…gnized

sourcery-ai · 2024-12-03T19:34:14Z

Reviewer's Guide by Sourcery

This PR updates the pipeline validation logic to be more flexible by allowing unrecognized pipeline names and versions in the processing status file. Instead of failing immediately when encountering unrecognized entries, the system now emits warnings and continues processing with the recognized pipelines/versions, only erroring out if no valid pipeline-version combinations are found.

Class diagram for updated pipeline validation logic

classDiagram
    class DerivativeUtils {
        +list get_recognized_pipelines(Iterable~str~ pipelines)
        +tuple~list, list~ classify_pipeline_versions(str pipeline, Iterable~str~ versions)
        +void check_at_least_one_pipeline_version_is_recognized(pd.DataFrame status_df)
    }

    class Mappings {
        +dict KNOWN_PIPELINE_URIS
        +dict KNOWN_PIPELINE_VERSIONS
    }

    DerivativeUtils --> Mappings : uses

    class CLI {
        +void derivatives(pd.DataFrame status_df)
    }

    CLI --> DerivativeUtils : uses

File-Level Changes

Change	Details	Files
Refactored pipeline validation logic to handle unrecognized entries	Split pipeline validation into separate functions for checking names and versions Added function to classify pipeline versions as recognized or unrecognized Added check to ensure at least one valid pipeline-version combination exists Modified pipeline completion check to only process recognized pipeline-version pairs	`bagel/utilities/derivative_utils.py` `bagel/cli.py`
Added comprehensive test coverage for new validation behavior	Added test for handling unrecognized pipelines with warnings Added test for error case when no valid pipeline-version combinations exist Updated existing pipeline version tests to use new classification approach	`tests/integration/test_cli_derivatives.py` `tests/unit/test_derivative_utils.py` `tests/data/proc_status_no_recognized_pipelines.tsv` `tests/data/proc_status_unrecognized_pipelines.tsv`

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#382	Allow derivatives command to proceed when at least one pipeline/version combination is recognized, emitting warnings instead of errors for unrecognized ones	✅

Possibly linked issues

Emit warning instead of erroring out when pipeline/version not recognized #382: PR updates handling of unrecognized pipelines, directly addressing the issue's error concern.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

codecov · 2024-12-03T19:34:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.56%. Comparing base (8578e4c) to head (5174bc6).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   98.42%   98.56%   +0.14%     
==========================================
  Files          18       18              
  Lines        1016     1116     +100     
==========================================
+ Hits         1000     1100     +100     
  Misses         16       16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sourcery-ai

Hey @alyssadai - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 2 issues found
🟢 Security: all looks good
🟡 Testing: 3 issues found
🟡 Complexity: 1 issue found
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

bagel/utilities/derivative_utils.py

tests/unit/test_derivative_utils.py

bagel/utilities/derivative_utils.py

sourcery-ai

Hey @alyssadai - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 2 issues found
🟡 Complexity: 1 issue found
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tests/unit/test_derivative_utils.py

tests/integration/test_cli_derivatives.py

bagel/utilities/derivative_utils.py

tests/integration/test_cli_derivatives.py

bagel/utilities/derivative_utils.py

alyssadai · 2024-12-04T03:56:36Z

@sourcery-ai review

sourcery-ai

Hey @alyssadai - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟡 Complexity: 1 issue found
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

bagel/utilities/derivative_utils.py

tests/integration/test_cli_derivatives.py

bagel/utilities/derivative_utils.py

bagel/mappings.py

bagel/utilities/derivative_utils.py

tests/unit/test_derivative_utils.py

tests/integration/test_cli_derivatives.py

surchs

Thanks @alyssadai for the PR. Looks like sourcery was trying to filibuster your work here - can't let that happen!

I have some clarity points so not approving yet, but they are pretty small. Mainly

what's most helpful to show user if they have bad pipeline names or versions
make tests easier for human brain

lmk what you think

surchs

edit: not sure where this ghost review is from 👀

surchs

Hey @alyssadai, I don't see any changes since the last review. Have you maybe not pushed your changes yet?

alyssadai · 2024-12-09T16:54:58Z

Hey @surchs, you're totally right, sorry about that! You should be able to see the changes now.

surchs

Looks great @alyssadai!

🧑‍🍳

neurobagel-bot · 2024-12-11T01:42:34Z

🚀 PR was released in v0.3.5 🚀

alyssadai added 6 commits December 3, 2024 01:27

update type hint

060da32

refactor utils to include check if all pipeline names/vers are unreco…

0cf56d8

…gnized

rename utility functions

90987b6

update docstrings

b9ca039

update tests of unrecognized pipeline names, fix warning msg

ca01e9c

update tests of pipeline version check

3e2481c

alyssadai added the pr-patch Incremental feature improvement, will increment patch version when merged (0.0.+1) label Dec 3, 2024

alyssadai marked this pull request as draft December 3, 2024 19:34

sourcery-ai bot reviewed Dec 3, 2024

View reviewed changes

alyssadai added 6 commits December 3, 2024 21:49

add e2e test of proc status file with some unrecognized pipelines

8a54374

fix message formatting

37220a9

test error when no pipeline-versions recognized

b85e5a1

fix assertion

cd92b51

remove TODO

2f031f1

add new test processing status TSVs

ebc8186

alyssadai marked this pull request as ready for review December 4, 2024 03:41

Merge branch 'main' into allow-unrecog-pipelines

64174cf

sourcery-ai bot reviewed Dec 4, 2024

View reviewed changes

alyssadai added 3 commits December 3, 2024 22:44

update test data README

1c8a4d3

simplify conditionals

a5ff5b4

merge if statements

de86836

sourcery-ai bot reviewed Dec 4, 2024

View reviewed changes

simplify conditionals

0aad5ec

surchs self-requested a review December 4, 2024 17:40

surchs approved these changes Dec 4, 2024

View reviewed changes

surchs reviewed Dec 4, 2024

View reviewed changes

alyssadai added the release Create a release when this PR is merged label Dec 6, 2024

alyssadai added 4 commits December 6, 2024 14:09

update message template for unrecognized pipelines

4294040

rename util func

87945fb

switch to boolean flag for recognized vers

0581446

make unrecognized pipeline versions clearer

789d493

alyssadai requested a review from surchs December 6, 2024 21:23

surchs reviewed Dec 9, 2024

View reviewed changes

alyssadai added 2 commits December 9, 2024 11:49

update test parameter names

ddedf47

switch order of asserts

5174bc6

alyssadai requested a review from surchs December 10, 2024 20:26

surchs approved these changes Dec 11, 2024

View reviewed changes

alyssadai merged commit f68468a into main Dec 11, 2024
9 checks passed

alyssadai deleted the allow-unrecog-pipelines branch December 11, 2024 01:42

neurobagel-bot bot added the released This issue/pull request has been released. label Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Allow unrecognized pipeline names and versions in processing status file #401

[ENH] Allow unrecognized pipeline names and versions in processing status file #401

alyssadai commented Dec 3, 2024 •

edited by surchs

Loading

sourcery-ai bot commented Dec 3, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

codecov bot commented Dec 3, 2024 •

edited

Loading

sourcery-ai bot left a comment

sourcery-ai bot left a comment

alyssadai commented Dec 4, 2024

sourcery-ai bot left a comment

surchs left a comment •

edited

Loading

surchs left a comment •

edited

Loading

surchs left a comment

alyssadai commented Dec 9, 2024

surchs left a comment

neurobagel-bot bot commented Dec 11, 2024

[ENH] Allow unrecognized pipeline names and versions in processing status file #401

[ENH] Allow unrecognized pipeline names and versions in processing status file #401

Conversation

alyssadai commented Dec 3, 2024 • edited by surchs Loading

Checklist

Summary by Sourcery

Summary by Sourcery

sourcery-ai bot commented Dec 3, 2024 • edited Loading

Reviewer's Guide by Sourcery

Class diagram for updated pipeline validation logic

File-Level Changes

Assessment against linked issues

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

codecov bot commented Dec 3, 2024 • edited Loading

Codecov Report

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot left a comment

Choose a reason for hiding this comment

alyssadai commented Dec 4, 2024

sourcery-ai bot left a comment

Choose a reason for hiding this comment

surchs left a comment • edited Loading

Choose a reason for hiding this comment

surchs left a comment • edited Loading

Choose a reason for hiding this comment

surchs left a comment

Choose a reason for hiding this comment

alyssadai commented Dec 9, 2024

surchs left a comment

Choose a reason for hiding this comment

neurobagel-bot bot commented Dec 11, 2024

alyssadai commented Dec 3, 2024 •

edited by surchs

Loading

sourcery-ai bot commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading

surchs left a comment •

edited

Loading

surchs left a comment •

edited

Loading