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

feat!: deprecate forget_length, add parameters to typetracer_with_report #2671

Merged

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Aug 25, 2023

  1. Deprecation of forget_length:
    This argument represents a leak of the implementation details; the function only accepts a form, which has no known length.

    • Add deprecation notice to wiki

    In future, we could add a *, length=unknown_length keyword-only argument that would be more powerful, but I can't see any examples of people needing that right now.

  2. Extend typetracer_with_report parameters
    Most uses of our typetracer_with_report function immediately wrap the result with an ak.Array. We should just support that, given that the highlevel argument is so widely used in our L1 API. Although typetracer isn't exactly L1, in my view L1-L2 is more about who uses these functions, rather than how painful they can be to use. @jpivarski this might reflect a changing view of mine, I am not sure. Please weigh in you think we should keep these functions "simple".

@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #2671 (ad0fe30) into main (3cd54fa) will decrease coverage by 0.01%.
The diff coverage is 73.33%.

Additional details and impacted files
Files Changed Coverage Δ
src/awkward/operations/ak_from_buffers.py 93.47% <ø> (ø)
src/awkward/typetracer.py 84.61% <73.33%> (-9.68%) ⬇️

@agoose77 agoose77 changed the title feat!: deprecate the forget_length argument of typetracer_with_report feat!: deprecate forget_length, add parameters to typetracer_with_report Aug 25, 2023
@agoose77 agoose77 temporarily deployed to docs-preview August 25, 2023 09:25 — with GitHub Actions Inactive
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.

👍

@agoose77 agoose77 force-pushed the agoose77/feat!-deprecate-forget-length-typetracer-with-report branch from 18bdc1d to aa144f8 Compare August 25, 2023 15:07
@agoose77 agoose77 enabled auto-merge (squash) August 25, 2023 15:07
@agoose77 agoose77 disabled auto-merge August 25, 2023 15:13
@agoose77
Copy link
Collaborator Author

@jpivarski could you sign-off on 624634f and ad0fe30 which add ak.typetracer.typetracer_from_form? I added this in ak.typetracer only, rather than ak._nplikes.typetracer because it's a public-only function. In time, that might also be true of _typetracer_with_report, but I decided not to move that yet.

@jpivarski
Copy link
Member

I think it looks like a good idea. I looked it over while we were talking—splitting my attention, but it was on the same topic—and I can see the usefulness of having a function like this. It's public, but I think it was motivated by an external project. (Was it Tiled? Coffea? I just tried to confirm where I got this impression, but wasn't able to find direct evidence.)

@agoose77
Copy link
Collaborator Author

Both coffea and dask-awkward would benefit from a function like this (see dask-contrib/dask-awkward#351 for an initial cleanup)

@agoose77 agoose77 enabled auto-merge (squash) August 27, 2023 12:41
@agoose77 agoose77 merged commit dea0139 into main Aug 27, 2023
34 checks passed
@agoose77 agoose77 deleted the agoose77/feat!-deprecate-forget-length-typetracer-with-report branch August 27, 2023 12:47
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.

None yet

2 participants