Prevent internal usage of expensive APIs

[vyasr]

This PR introduces a decorator that can be used to mark APIs that are unnecessarily expensive and should be avoided in internal usage. The decorator does nothing by default, but when the corresponding environment variable is set the decorator wraps decorated functions in a check of the current call stack. If the caller of that API is within the cudf source tree, it will raise an exception that may optionally also indicate an alternate API for developers to use instead. This implementation is completely transparent to users (including having no performance impact aside from some negligible additional import time from applying the decorator) but can be an invaluable tool to developers seeking to reduce Python overheads in cuDF Python. This decorator has been tested and shown to work (both locally and on CI) for DataFrame.columns, but those changes are not included in this PR in order to keep the changeset clean.

[codecov]

Codecov Report

Merging #10263 (cc72147) into branch-22.04 (a7d88cd) will increase coverage by 0.23%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.04   #10263      +/-   ##
================================================
+ Coverage         10.42%   10.66%   +0.23%     
================================================
  Files               119      122       +3     
  Lines             20603    20898     +295     
================================================
+ Hits               2148     2228      +80     
- Misses            18455    18670     +215     

Impacted Files	Coverage Δ
python/cudf/cudf/_fuzz_testing/fuzzer.py	0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/io.py	0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/main.py	0.00% <ø> (ø)
python/cudf/cudf/_version.py	0.00% <ø> (ø)
python/cudf/cudf/comm/gpuarrow.py	0.00% <ø> (ø)
python/cudf/cudf/core/_base_index.py	0.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py	0.00% <ø> (ø)
python/cudf/cudf/core/column/column.py	0.00% <ø> (ø)
python/cudf/cudf/core/column/datetime.py	0.00% <ø> (ø)
python/cudf/cudf/core/column/methods.py	0.00% <ø> (ø)
... and 62 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f263820...cc72147. Read the comment docs.

[shwina]

This is cool!

I'm wondering about its applicability beyond .columns though -- are there a significant number of other user-facing APIs that we both (1) tend to use internally, and (2) can easily replace with a call to an internal API?

[vyasr]

This is cool!

I'm wondering about its applicability beyond .columns though -- are there a significant number of other user-facing APIs that we both (1) tend to use internally, and (2) can easily replace with a call to an internal API?

That's a great question. I don't anticipate this being terribly broadly applicable, but there are enough use cases for it to be valuable I think. A couple that come to mind:

• DataFrame.dtypes generates a pandas.Series and I created a simpler internal equivalent DataFrame._dtypes a while ago that just provides a dictionary. Here's the relative performance:

In [3]: %timeit df1.dtypes
129 µs ± 636 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

In [4]: %timeit df1._dtypes
1.37 µs ± 10.5 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

• The constructors of all Frame types should almost never be used internally. I'm not sure we can forbid these entirely since there may be special cases where we need to support odd user-provided inputs, but at minimum this new decorator could be used locally to identify cases where we could instead use _from_data or something like it (which we know is much faster).
• The user-facing join API is very expensive, but we still use it internally for index alignment all over the place. Addressing [FEA] Refactor cuDF Python merging logic for index types #9978 will help reduce the cost, but even with that I suspect that we will want to write a simpler internal-facing API.
• Similarly, I think we will want to define a simpler internal version of concat, which is extremely complex. @galipremsagar can probably speak to how frequently it shows up in profiles. There is a lot of extra logic there (similar to joins) that could probably be excised from a simpler internal implementation.

It's possible (I would even say likely...) that there are other slow APIs that we just aren't aware of right now and might turn up upon more careful benchmarking, or APIs that seem unavoidable internally but might actually be more avoidable than we think. For instance, could we get rid of DataFrame.__getitem__ calls entirely? I doubt it, but I think we can at least replace a lot of them with calls to df._data.__getitem__, which is much faster.

[brandon-b-miller]

This is cool!

I'm wondering about its applicability beyond .columns though -- are there a significant number of other user-facing APIs that we both (1) tend to use internally, and (2) can easily replace with a call to an internal API?

Another common one is use of DataFrame.__getitem__ when looping through a dataframe columns rather than accessing via _data.

[quasiben]

Setting the env var for devs could be done with conda env.yml file:
https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#setting-environment-variables

[vyasr]

Setting the env var for devs could be done with conda env.yml file: https://docs.conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html#setting-environment-variables

Thanks for the find @quasiben. I ended up going with a pytest-based approach so that tests will always be run this way.

[bdice]

Looks good. Note for the future: I prefer to review PRs like this with an example for context. I would have appreciated seeing this applied to a single function so that I could better rationalize about the design. I saw your comment about it being tested and wanting to keep the changeset clean, but as a reviewer I can't prove that this works as claimed unless it is used at least once.

As a middle ground of "clean changeset" and "design in context," you could include a code example in the PR description or in a comment instead of in the changeset. For this specific case, the decorator is fairly trivial, though, so not worth worrying about an example.

[vyasr]

Looks good. Note for the future: I prefer to review PRs like this with an example for context. I would have appreciated seeing this applied to a single function so that I could better rationalize about the design. I saw your comment about it being tested and wanting to keep the changeset clean, but as a reviewer I can't prove that this works as claimed unless it is used at least once.

As a middle ground of "clean changeset" and "design in context," you could include a code example in the PR description or in a comment instead of in the changeset. For this specific case, the decorator is fairly trivial, though, so not worth worrying about an example.

Unfortunately you're just a bit late to the game. This branch originally did contain an example of applying this to DataFrame.columns and we discussed these changes at some length in our team meeting last week. I only rebased those changes out this morning because in the meeting we agreed that it would be useful to be have this changeset be completely independent in the event that we decide to undo it for some reason. I probably should have removed your request for review and requested one of the others who had a chance to review the original version of this PR, I agree that this is tricky tricky to review in a vacuum. Fortunately @brandon-b-miller (the other requested reviewer) did see and comment on the original design. If you don't feel confident in your review, please request an additional review from someone who commented above since they should have had a chance to at least skim the code with the example.

[bdice]

Haha, that's fine then! I didn't know the context, my apologies. I definitely feel confident enough in the PR to count my approval here.

[ajschmidt8]

Approving ops-codeowner file changes

[vyasr]

@gpucibot merge