Skip to content
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.

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

Add debug flag and architecture error message #80

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Feb 16, 2024

This PR adds two usability features for the CLI

  1. By default the usual error traceback to the code is disabled, but the error original message is still printed. One can enable the traceback again by using the new --debug flag.
  2. Wrap all calls to an architecture with custom try-except block. If an error is raised in this block we reraise the error with extra message that this error is most likely not our fault.

📚 Documentation preview 📚: https://metatensor-models--80.org.readthedocs.build/en/80/

@PicoCentauri PicoCentauri force-pushed the debug-traceback branch 2 times, most recently from 691fbf5 to e8a291a Compare February 16, 2024 16:29
Comment on lines 18 to 19
f"{exception}\n\nThis error originates from an architecture, and is likely "
"not a problem with metatensor-models."
Copy link
Collaborator

@frostedoyster frostedoyster Feb 16, 2024

Choose a reason for hiding this comment

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

I feel this will make our users think they're dealing with a collection of models rather
than a piece of useful software (and therefore will not give us credit)

Suggested change
f"{exception}\n\nThis error originates from an architecture, and is likely "
"not a problem with metatensor-models."
f"{exception}\n\nThis error originates from an architecture. If you think this is "
"a bug, you can contact its maintainer (see the architecture's documentation)."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is a better. I will change it and also swap the order. First the we give the test and below state the error message.



def test_architecture_error():
with pytest.raises(ArchitectureError, match="not a problem with metatensor-models"):
Copy link
Collaborator

@frostedoyster frostedoyster Feb 16, 2024

Choose a reason for hiding this comment

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

Suggested change
with pytest.raises(ArchitectureError, match="not a problem with metatensor-models"):
with pytest.raises(ArchitectureError, match="you can contact its maintainer"):

@PicoCentauri PicoCentauri force-pushed the debug-traceback branch 2 times, most recently from bad7d08 to 5a607ad Compare February 19, 2024 09:37
@PicoCentauri PicoCentauri force-pushed the debug-traceback branch 2 times, most recently from d0d718b to f48faab Compare February 20, 2024 08:31
@PicoCentauri
Copy link
Contributor Author

Can you take another final look @frostedoyster. I saw that a couple of tests are basically not testing the output of an error raise. sorry.

@frostedoyster
Copy link
Collaborator

Thanks a lot!

@frostedoyster frostedoyster merged commit f2c6899 into main Feb 20, 2024
9 checks passed
@frostedoyster frostedoyster deleted the debug-traceback branch February 20, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants