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

Better error messages #15

Merged
merged 11 commits into from
Apr 23, 2024
Merged

Conversation

philtweir
Copy link
Contributor

@philtweir philtweir commented Apr 15, 2024

Implements several closely related features:

  • error messages show location of call, not lazy-evaluation, if possible
  • custom type errors are more specific
  • examples in the custom error messages

@philtweir philtweir linked an issue Apr 15, 2024 that may be closed by this pull request
@philtweir philtweir changed the base branch from main to release/0.9.1 April 15, 2024 17:38
@philtweir
Copy link
Contributor Author

Couple of caveats:

  • the non-local traceback implementation was hard, so brings a risk of brittleness - we should keep an eye for any related issues
  • only the custom type errors have been improved (other than the non-local context), not the native ones - improving Python's standard way of reporting a missing argument seems out-of-scope for this package, and not "least surprise", but the custom errors have generally been given more detail
  • as above, the standard errors have been left alone, but the couple of custom errors where this might help have been given inline examples

@philtweir philtweir changed the title [Stacked on #14] Better error messages Better error messages Apr 19, 2024
@philtweir philtweir changed the base branch from release/0.9.1 to main April 19, 2024 20:50
@philtweir philtweir changed the base branch from main to release/0.9.1 April 19, 2024 20:50
tests/test_errors.py Outdated Show resolved Hide resolved
tests/test_errors.py Outdated Show resolved Hide resolved
src/dewret/tasks.py Outdated Show resolved Hide resolved
tests/test_errors.py Outdated Show resolved Hide resolved
tests/test_errors.py Outdated Show resolved Hide resolved
tests/test_errors.py Outdated Show resolved Hide resolved
tests/test_errors.py Outdated Show resolved Hide resolved
src/dewret/tasks.py Outdated Show resolved Hide resolved
Captures a traceback, for debugging if this does not work.

WARNING: this is one of the few places that we would expect
dask distributed to break, if running outside a single process
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder about this warning. As far as I know dask.distributed is not a dependency of ours here. I can see warning/pointing out failure points for future developers (including our future selves) thinking of extending this package for parallel execution.

Separately, the internal lazy construct is supposed to be library agnostic (so in principle users can use dask, ray, etc backends), so should we perhaps make the warning library agnostic as well?

Copy link
Contributor

@elleryames elleryames left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @philtweir !

@philtweir philtweir merged commit 7c1439a into release/0.9.1 Apr 23, 2024
13 checks passed
@KamenDimitrov97 KamenDimitrov97 mentioned this pull request Aug 7, 2024
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.

Better error messages [1-40,41]
2 participants