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

Do not create conda error reports for common / expected exceptions #5264

Merged
merged 10 commits into from
May 22, 2024

Conversation

jaimergp
Copy link
Contributor

@jaimergp jaimergp commented Mar 28, 2024

Description

Closes #5263

This is what we would need to close that issue without registering new base classes in conda exception handler.

CondaError calls super() so we can't compose new exception types that inherit both from CondaError and e.g. CalledProcessError. This prevents us from having a mixed exception that doesn't change which type of exception is raised (for the purpose of try/except idioms or isinstance checks).

I'd rather have this:

class BuildScriptException(CondaError, CalledProcessError):
  ...

and then call:

try:
  subprocess_for_build_script()
except CalledProcessError from exc:
  raise BuildScriptException(exc, ...) from exc

Achieving this ^ will require changes in conda/conda though (either changing the CondaError class so it doesn't call super(), or having a different base class for the purpose of exception handling like BaseCondaError).

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@jaimergp jaimergp requested a review from a team as a code owner March 28, 2024 14:23
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Mar 28, 2024
@jaimergp jaimergp changed the title Error gracefully Do not create conda error reports for common / expected exceptions Mar 28, 2024
Copy link

codspeed-hq bot commented Mar 28, 2024

CodSpeed Performance Report

Merging #5264 will not alter performance

Comparing jaimergp:error-gracefully (d870fed) with main (9c0ceec)

Summary

✅ 3 untouched benchmarks

@jaimergp
Copy link
Contributor Author

pre-commit.ci autofix

@jezdez
Copy link
Member

jezdez commented Apr 29, 2024

@kenodegard @jaimergp How does this overlap with #5255?

@jaimergp
Copy link
Contributor Author

jaimergp commented Apr 29, 2024

Mostly compatible. We need to agree on a base exception for all conda-build exceptions, or just subclass CondaError for each one. I chose the former, but Ken's does:

class CondaBuildUserError(CondaError):
  ...

vs my idea:

class CondaBuildError(CondaError):
  ...

class CondaBuildUserError(CondaBuildError):
  ...

I don't mind either, but we need to agree on one.

kenodegard
kenodegard previously approved these changes May 21, 2024
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

I don't mind either with how we define CondaBuildUserError

I only opted for CondaBuildUserError(CondaError) to avoid any potential side-effects of inheriting directly from CondaBuildError 🤷🏼‍♂️

We've also decided it would be better to split #5255 into smaller PRs for easier review and testing so merging this ahead of reworking all of that would be good

tests/test_api_build.py Show resolved Hide resolved
tests/test_api_build.py Outdated Show resolved Hide resolved
@kenodegard kenodegard enabled auto-merge (squash) May 22, 2024 18:00
@@ -1945,7 +1946,7 @@ def test_add_pip_as_python_dependency_from_condarc_file(
testing_metadata, testing_workdir, add_pip_as_python_dependency, monkeypatch
):
"""
Test whether settings from .condarc files are heeded.
Test whether settings from .condarc files are needed.
Copy link
Member

@beeankha beeankha May 22, 2024

Choose a reason for hiding this comment

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

Suggested change
Test whether settings from .condarc files are needed.
Test whether settings from .condarc files are heeded.

I think this wording edit needs to be reverted, since this test seems to be checking whether or not the settings in the .condarc files are actually being taken into account/paid attention to (?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Long conda info error messages for common exceptions
5 participants