Skip to content

Drop __firstlineno__ on ProxyBase instances - #8045

Merged
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
jcrist:drop-firstlineno
May 4, 2026
Merged

Drop __firstlineno__ on ProxyBase instances#8045
rapids-bot[bot] merged 1 commit into
NVIDIA:mainfrom
jcrist:drop-firstlineno

Conversation

@jcrist

@jcrist jcrist commented May 4, 2026

Copy link
Copy Markdown
Member

We don't want to define __firstlineno__ on instances of ProxyBase.

  • We don't want to proxy through the original __firstlineno__, since we've updated __module__, so the line numbers would no longer be accurate.
  • We don't want to use our __firstlineno__ for the same reason - they map to a different file than __module__.

Dropping this attribute causes tools like inspect.getsourcelines to error appropriately, saying the source code cannot be found. Note that any methods inspected on our proxy classes will still report the original sklearn code (as intended), it's just grabbing the whole class that won't work.

The main place this shows up is some docstring tests in the sklearn repo - previously these were implicitly skipped because getsourcelines would error. The recent metaclass changes led to this not erroring in all python versions, leading to failures in our nightlies.

This PR fixes things so that we drop __firstlineno__ uniformly, leading to the tests to pass/skip in all python versions, reverting to the previous behavior.

We don't want to define `__firstlineno__` on instances of `ProxyBase`.

- We don't want to proxy through the original `__firstlineno__`, since
  we've updated `__module__`, so the line numbers would no longer be
  accurate.
- We don't want to use our `__firstlineno__` for the same reason - they
  map to a different file than `__module__`.

Dropping this attribute causes tools like `inspect.getsourcelines` to
error appropriately, saying the source code cannot be found. Note that
any _methods_ inspected on our proxy classes will still report the
original sklearn code (as intended), it's just grabbing the whole class
that won't work.

The main place this shows up is some docstring tests in the sklearn repo
- previously these were implicitly skipped because `getsourcelines`
would error. The recent metaclass changes led to this not erroring in
_all_ python versions, leading to failures in our nightlies.

This PR fixes things so that we drop `__firstlineno__` uniformly,
leading to the tests to pass/skip in all python versions, reverting to
the previous behavior.
@jcrist jcrist self-assigned this May 4, 2026
@jcrist
jcrist requested a review from a team as a code owner May 4, 2026 16:46
@jcrist
jcrist requested a review from divyegala May 4, 2026 16:46
@jcrist jcrist added bug Something isn't working non-breaking Non-breaking change cuml-accel Issues related to cuml.accel labels May 4, 2026
@github-actions github-actions Bot added the Cython / Python Cython or Python issue label May 4, 2026
@coderabbitai

coderabbitai Bot commented May 4, 2026

Copy link
Copy Markdown
📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Fixed proxy class metadata initialization to properly handle class properties and attributes when modules are reassigned during the creation process. This correction prevents potential metadata inconsistencies that could negatively impact proxy class behavior within the acceleration module, improving system stability and ensuring consistent performance of the proxy mechanism across different operational scenarios.

Walkthrough

The PR modifies ProxyBase.__init_subclass__ in the estimator proxy module to remove the __firstlineno__ attribute from generated proxy classes, preventing the original class's line number metadata from persisting after __module__ reassignment.

Changes

Proxy Class Metadata Cleanup

Layer / File(s) Summary
Proxy Metadata Handling
python/cuml/cuml/accel/estimator_proxy.py
ProxyBase.__init_subclass__ adds a try/except block to delete __firstlineno__ from the proxy class, ensuring generated classes do not retain the original class's line number.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested labels

Cython / Python

Suggested reviewers

  • csadorf
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: removing __firstlineno__ from ProxyBase instances.
Description check ✅ Passed The description clearly explains the rationale, implementation details, and impact of removing __firstlineno__ from ProxyBase instances.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Review rate limit: 9/10 reviews remaining, refill in 6 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/cuml/cuml/accel/estimator_proxy.py`:
- Around line 246-257: Add a regression test that verifies deletion of
__firstlineno__ on proxy classes: create a proxy class instance (the class
produced by the estimator proxy metaclass in estimator_proxy.py) and assert that
inspect.getsource(proxy_class) raises (e.g., OSError) while
inspect.getsource(proxy_class.some_method) or
inspect.getsource(proxy_instance.some_method) still returns the method source;
this ensures class-level source inspection fails after del cls.__firstlineno__
but method-level lookup remains intact. Include the test alongside existing
estimator_proxy tests and reference the proxy class created by the metaclass and
the cls.__firstlineno__ behavior in the test name/comment.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 48bbe255-af8c-4159-8677-f4289be825b2

📥 Commits

Reviewing files that changed from the base of the PR and between d8e41af and afde88d.

📒 Files selected for processing (1)
  • python/cuml/cuml/accel/estimator_proxy.py

Comment thread python/cuml/cuml/accel/estimator_proxy.py
@jcrist

jcrist commented May 4, 2026

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit 3587a1b into NVIDIA:main May 4, 2026
103 checks passed
@jcrist
jcrist deleted the drop-firstlineno branch May 4, 2026 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cuml-accel Issues related to cuml.accel Cython / Python Cython or Python issue non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants