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

docs: how to accelerate awkward arrays with cppyy #2334

Merged
merged 22 commits into from
Nov 14, 2023

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Mar 22, 2023

This PR adds:

  • a Jupyter notebook as documentation
  • bug fix: generating an ArrayView when requesting the array C++ type

See #2334 (comment) for information on the CI/CD changes.

TODO

  • add redirects to documentation for regrouped pages.

@ianna ianna temporarily deployed to docs-preview March 22, 2023 07:50 — with GitHub Actions Inactive
@ianna ianna marked this pull request as draft March 22, 2023 12:45
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 - please, suggest what is the best place for Awkward-cppyy JIT. Thanks.

docs/_toc.yml Outdated Show resolved Hide resolved
docs/_toc.yml Outdated Show resolved Hide resolved
@ianna ianna temporarily deployed to docs-preview March 22, 2023 14:27 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Merging #2334 (60e9f8f) into main (ad04fc7) will not change coverage.
Report is 3 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@ianna ianna temporarily deployed to docs-preview March 22, 2023 14:50 — with GitHub Actions Inactive
@ianna ianna marked this pull request as ready for review March 22, 2023 15:01
@ianna ianna requested a review from jpivarski March 22, 2023 15:01
@agoose77
Copy link
Collaborator

agoose77 commented Mar 22, 2023

@ianna I think creating a new section was a good decision. I've renamed it to how-to-use-in-cpp, because I think we should also move the LayoutBuilder documentation here? If I understand correctly, we should be able to use LayoutBuilder in cppyy.

If you and @jpivarski agree, then I won't yet move the layout-builder documentation here; I'll make a new PR for that.

@agoose77 agoose77 temporarily deployed to docs-preview March 22, 2023 15:33 — 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.

This has the same fix in it as #2335, so it should be merged afterward.

Also, it should be merged after cppyy 3.0.1 is released (watch this space) and converted into Jupytext that gets evaluated along with the other docs (that's the requested change). The rationale is that we shouldn't put documentation out there for a feature that users can't use... without manually installing cppyy from GitHub and compiling it for half an hour...

The escape valve to allow unevaluated ipynb docs should only be used for GPU docs, since they have to avoid the normal mechanism. For a library that isn't released yet, we can just wait for it to be released. This PR will be easy to merge when that happens.

@agoose77 agoose77 added the pr-on-hold This PR is inactive due to a pending decision or other constraint label Mar 22, 2023
@agoose77 agoose77 force-pushed the ianna/user-guide-cppyy-awkward-git branch from 13d39a8 to 397c320 Compare November 12, 2023 00:13
@agoose77
Copy link
Collaborator

agoose77 commented Nov 12, 2023

Well, that took some figuring out. I wrote a build script to run inside a manylinux container, which is used to generate the cppyy wheels. This allows to build a separate container that acts as a binary store for the built wheels, which are then extracted in our CI and installed.

For the tests, we install the wheels in a dedicated non-ROOT cppyy job. For the docs, we have a dedicated build-the-cppyy-notebook job that does a similar thing.

This is a somewhat ugly procedure: I'd prefer to push the wheels to a PyPI registry, but off the top of my head I can't think of any non PyPI-registries that we can easily use (though we could push to an AWS bucket).

Once cppyy is released, we should remove this monstrosity.

In this PR I also attempted to upgrade our pyodide toolchain. Unfortunately, there are yet more out-of-tree bugs in pyodide whose fixes have yet to be released. So, that's on the bench for a while.

@agoose77 agoose77 enabled auto-merge (squash) November 12, 2023 23:29
@agoose77 agoose77 removed the pr-on-hold This PR is inactive due to a pending decision or other constraint label Nov 12, 2023
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 - thanks for looking into it! I've already forgotten about this PR :-)

@agoose77
Copy link
Collaborator

agoose77 commented Nov 13, 2023

@jpivarski I've added a new section to the user guide:

image

I've not renamed the actual file for the existing LayoutBuilder docs; this means that we don't need to rewrite the URLs for existing hyperlinks in the wild.

Once I've checked the deployment, I'll merge if you give this a 👍

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.

Fantastic! We're in a slightly odd position of having documentation for this feature and testing it in CI, but users still can't install the right version of cppyy to use it. I noticed that my comment holding it back was waiting on cppyy 3.0.1 since March. But let's get the documentation out now so that it's ready when cppyy is.

When the easily installable version of cppyy does come out, though, we'll want to use that. Perhaps it won't have these warning messages...

image

This is ready to merge, though, as-is.

docs/user-guide/how-to-use-in-cpp-cppyy.ipynb Show resolved Hide resolved
@agoose77 agoose77 enabled auto-merge (squash) November 14, 2023 10:31
@agoose77 agoose77 merged commit f229c47 into main Nov 14, 2023
38 checks passed
@agoose77 agoose77 deleted the ianna/user-guide-cppyy-awkward-git branch November 14, 2023 10:31
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