Skip to content

feat: container error handling #275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
agitter merged 22 commits into from
Oct 11, 2025
Merged

feat: container error handling #275

agitter merged 22 commits into from
Oct 11, 2025

Conversation

@tristan-f-r
Copy link
Collaborator

@tristan-f-r tristan-f-r commented Jun 13, 2025 •
edited
Loading

Closes #119, closes #378. This PR introduces the new general ContainerError to wrap all potential container handlers from now on.

DOMINO and OI2 are demonstrated using this error by searching for a specific error, or else bubbling the error up if no predicted error happened.

Two notes:

  • No method should test for ContainerErrors happening - each one should be properly handled before run ever gets called, or inside run.
  • String matching is fallible - a malicious user could hide an error by making a gene name one of the errors and looking for an error that prints out the gene name. This code assumes that this scenario is implausible.

@tristan-f-r
feat: container error handling
62009eb 
closes Reed-CompBio#21, closes Reed-CompBio#119, closes Reed-CompBio#218. This PR introduces the new general ContainerError to wrap all potential container handlers from now on.

DOMINO and OI2 are demonstrated using this error by searching for a specific error, or else bubbling the error up if no predicted error happened.
tristan-f-r
tristan-f-r commented Jun 13, 2025
View reviewed changes
@tristan-f-r tristan-f-r marked this pull request as draft June 14, 2025 00:00
@tristan-f-r

This comment was marked as outdated.

Sign in to view
@tristan-f-r tristan-f-r marked this pull request as ready for review June 16, 2025 16:32
@tristan-f-r tristan-f-r added the enhancement New feature or request label Jun 16, 2025
This was referenced Jun 23, 2025
Merged
Closed
@read-the-docs-community
Copy link

read-the-docs-community bot commented Aug 15, 2025 •
edited
Loading

Documentation build overview

📚 spras | 🛠️ Build #29903411 | 📁 Comparing 6873ee8 against latest (6e8cc04)


🔍 Preview build

Show files changed (7 files in total): 📝 6 modified | ➕ 0 added | ➖ 1 deleted
File Status
genindex.html 📝 modified
htcondor.html ➖ deleted
index.html 📝 modified
fordevs/modules.html 📝 modified
fordevs/spras.html 📝 modified
prms/domino.html 📝 modified
prms/prms.html 📝 modified

@tristan-f-r tristan-f-r mentioned this pull request Aug 20, 2025
Closed
agitter
agitter reviewed Aug 23, 2025
View reviewed changes
agitter
agitter reviewed Sep 5, 2025
View reviewed changes
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a lot of local test failures on this branch, but I haven't confirmed whether it is related to these changes or some other local problem.

@agitter
Copy link
Collaborator

agitter commented Sep 5, 2025

The core feature of container error handling is now ready. The two unresolved comments pertain to specific DOMINO behavior. We could keep this open to dig into them here or open a follow up issue to get this merged.

@github-actions github-actions bot added the merge-conflict This PR has merge conflicts. label Oct 3, 2025
@agitter
Copy link
Collaborator

agitter commented Oct 3, 2025

My last comment referred to "two unresolved comments pertain". One of those has an issue (#388). Is the other the need for a larger test case? If so, please create an issue. Then this should be ready to merge.

@github-actions github-actions bot removed the merge-conflict This PR has merge conflicts. label Oct 4, 2025
@tristan-f-r tristan-f-r mentioned this pull request Oct 8, 2025
Open
1 task
@tristan-f-r
Copy link
Collaborator Author

tristan-f-r commented Oct 8, 2025 •
edited
Loading

I've added a note on #235, which contains the larger dataset. We can also merge #235 first, then add the test here.

@tristan-f-r tristan-f-r requested a review from agitter October 8, 2025 20:11
agitter
agitter reviewed Oct 10, 2025
View reviewed changes
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test_node_precision_recall_pca_chosen_pathway test failed on the last commit. I don't see what would have affected that.

@tristan-f-r
Copy link
Collaborator Author

tristan-f-r commented Oct 11, 2025 •
edited
Loading

test_node_precision_recall_pca_chosen_pathway is a flaky test.

@tristan-f-r tristan-f-r requested a review from agitter October 11, 2025 00:50
agitter
agitter approved these changes Oct 11, 2025
View reviewed changes
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_node_precision_recall_pca_chosen_pathway is a flaky test.

To prove your point, it passed this time.

@agitter agitter merged commit fea328d into Reed-CompBio:main Oct 11, 2025
19 checks passed
@tristan-f-r tristan-f-r deleted the error-announce branch October 11, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntalluri ntalluri ntalluri left review comments

@agitter agitter agitter approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error running DOMINO with very small inputs Capturing errors thrown by algorithms run inside Docker containers

3 participants

@tristan-f-r @agitter @ntalluri