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

Revert "Add custom name setter and getter for proxy objects in cudf.pandas" #16267

Closed

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Jul 12, 2024

Reverts #16234

This is an investigation PR into sudden spike in failures.

@galipremsagar galipremsagar requested a review from a team as a code owner July 12, 2024 01:40
@github-actions github-actions bot added Python Affects Python cuDF API. cudf.pandas Issues specific to cudf.pandas labels Jul 12, 2024
@galipremsagar galipremsagar added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 12, 2024
Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-24.08@a6de6cc). Learn more about missing BASE report.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-24.08   #16267   +/-   ##
===============================================
  Coverage                ?   83.44%           
===============================================
  Files                   ?      184           
  Lines                   ?    28142           
  Branches                ?        0           
===============================================
  Hits                    ?    23482           
  Misses                  ?     4660           
  Partials                ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Matt711
Copy link
Contributor

Matt711 commented Jul 12, 2024

Hey @galipremsagar, do you know where I can see the failures caused by #16234?

additional_attributes={
"__init__": _DELETE,
"__setattr__": Index__setattr__,
"name": property(name),
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh maybe this should've been a _FastSlowAttribute?

return self._fsproxy_wrapped._name


def Index__setattr__(self, name, value):
Copy link
Contributor

@Matt711 Matt711 Jul 12, 2024

Choose a reason for hiding this comment

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

Just a note: This is the same __setattr__ method from _FastSlowProxy except customized to set the _name attr(_names for MultiIndex).

edit: add a link

@galipremsagar
Copy link
Contributor Author

Hey @galipremsagar, do you know where I can see the failures caused by #16234?

They can be seen in this job summary: https://github.com/rapidsai/cudf/actions/runs/9877265093/attempts/2#summary-27293125763

Since this PR reverts the changes that were causing the failures, you can see the same job summary of this PR here: https://github.com/rapidsai/cudf/actions/runs/9901041027/attempts/2#summary-27373925178

@Matt711
Copy link
Contributor

Matt711 commented Jul 12, 2024

They can be seen in this job summary: https://github.com/rapidsai/cudf/actions/runs/9877265093/attempts/2#summary-27293125763

Since this PR reverts the changes that were causing the failures, you can see the same job summary of this PR here: https://github.com/rapidsai/cudf/actions/runs/9901041027/attempts/2#summary-27373925178

Thanks for catching this! I wasn't aware of these tables. Its interesting that the drop is so large, so I'll look into some of the pandas tests with large drops in the table.

@Matt711
Copy link
Contributor

Matt711 commented Jul 12, 2024

I wonder how "volatile" the pandas pass rate is. What's the threshold for large decrease or increase?

@galipremsagar
Copy link
Contributor Author

I wonder how "volatile" the pandas pass rate is. What's the threshold for large decrease or increase?

0.01 to 0.03% changes can be safely considered noise.

@Matt711
Copy link
Contributor

Matt711 commented Jul 12, 2024

0.01 to 0.03% changes can be safely considered noise.

Any reason why we don't make the pandas test check fail for changes >0.05% (say)?

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Jul 12, 2024

0.01 to 0.03% changes can be safely considered noise.

Any reason why we don't make the pandas test check fail for changes >0.05% (say)?

We currently compare against the nightly baseline, which means we will have a delta that is not being caused by the PR alone, but the diff between the nightly and the PR. To fail the job if the changes >0.05% we will to have a baseline that is run on branch-24.08 and upto-date. To do that we are running short of CI resources. Hence the current approach.

@galipremsagar
Copy link
Contributor Author

Resolved by #16270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cudf.pandas Issues specific to cudf.pandas improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants