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

refactor: split error-handling code from _util #1752

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

agoose77
Copy link
Collaborator

ak._util has become something of a catch-all module. Besides making _util quite large, this also means that _util depends on many Awkward modules, which in turn depend upon _util. Currently, we resolve this by using the module object, which can break import cycles. However, this can be improved.

This PR splits out the error-handling code into an _errors private module.

There is more work that can be done here, but to keep this PR small and mergeable, we can do that in another PR.

@agoose77 agoose77 changed the title Agoose77/refactor smaller util refactor: split error-handling code from _util Sep 29, 2022
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #1752 (05054ba) into main (084fa4b) will increase coverage by 0.00%.
The diff coverage is 49.86%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_connect/avro.py 87.17% <0.00%> (ø)
src/awkward/_connect/cuda/__init__.py 0.00% <0.00%> (ø)
src/awkward/_connect/jax/__init__.py 90.47% <0.00%> (ø)
src/awkward/_connect/numexpr.py 89.85% <0.00%> (ø)
src/awkward/_connect/rdataframe/from_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/_connect/rdataframe/to_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/behaviors/mixins.py 97.50% <0.00%> (ø)
src/awkward/forms/bitmaskedform.py 78.72% <0.00%> (ø)
src/awkward/forms/bytemaskedform.py 78.16% <0.00%> (ø)
src/awkward/forms/emptyform.py 76.47% <0.00%> (ø)
... and 149 more

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

Yes, _util is a catch-all (because naming is hard, and I wanted to have a low barrier to making helper functions whenever we needed one). But this _errors is a good, well-defined classification.

The intention was for _util to be a dependency of most submodules, but _util itself doesn't depend on them (i.e. very early in the dependency graph). It looks like _util depends on nplikes at module-level, but all the other accesses of ak.* are inside functions—in other words, delayed—and therefore not a dependency cycle.

Similarly, nplikes only depends on ak.* inside functions, no dependency cycles when the module loads.

@jpivarski jpivarski merged commit 47ea26b into main Sep 29, 2022
@jpivarski jpivarski deleted the agoose77/refactor-smaller-util branch September 29, 2022 14:38
@agoose77
Copy link
Collaborator Author

agoose77 commented Sep 29, 2022

Yes, to clarify; we don't have any "broken" import cycles, but that is managed by using the two-phase module initialisation feature (and, if this weren't possible, we could still import other names within functions). Ideally, we could declare our dependencies at the top of the file with explicit imports, i.e. as a DAG. It won't always be possible to perform this split without compromising the general structure and layout of Awkward, and we don't need to pursue that level of separation.

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