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

fix: update cppyy module #2747

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Oct 8, 2023

cppyy module relied on ak._utils to check the version.
Also we need to import cling. I'm not sure this is the best place, but it works:

import awkward._connect.cling  # noqa: F401

@ianna ianna requested a review from jpivarski October 8, 2023 07:55
Copy link
Collaborator Author

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@agoose77 and @jpivarski - preparing for PyHEP I've discovered that cppyy test would fail. I wonder if this patch can be integrated in the next release asap. Thanks!

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #2747 (a7a327d) into main (c9aaacf) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

Additional details and impacted files
Files Coverage Δ
src/awkward/cppyy.py 27.27% <50.00%> (ø)
src/awkward/highlevel.py 76.72% <0.00%> (-0.12%) ⬇️

@agoose77
Copy link
Collaborator

agoose77 commented Oct 8, 2023

@ianna I presume we need to install cppyy from source to obtain the supported version? Perhaps we should add a CI job to do this.

When obtaining cppyy from Git, do you locally need to bump the version to 3.0.1?

@ianna
Copy link
Collaborator Author

ianna commented Oct 8, 2023

@ianna I presume we need to install cppyy from source to obtain the supported version?

yes

Perhaps we should add a CI job to do this.

When obtaining cppyy from Git, do you locally need to bump the version to 3.0.1?

yes

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, ak._util.parse_version was removed in 11e61b6#diff-5b25481afa0bba148a54d46dfc93af8141dfa50dcb399c55bb7df34f3af40f64

The dangerous-looking

import awkward._connect.cling

happens inside of the non-inspectable ak.Array.cpp_type property, so it's safe.

I think this is ready to go and will merge it.

@jpivarski jpivarski merged commit a6e426e into main Oct 9, 2023
37 checks passed
@jpivarski jpivarski deleted the ianna/update-cppyy-to-the-new-awkward-structure branch October 9, 2023 14:39
@agoose77
Copy link
Collaborator

agoose77 commented Oct 9, 2023

Thanks for merging this @jpivarski, I missed Ianna's reply!

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.

3 participants