-
Notifications
You must be signed in to change notification settings - Fork 890
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 warnings in remaining modules #12406
Conversation
…importing them direclty.
Codecov ReportBase: 86.58% // Head: 85.71% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-23.02 #12406 +/- ##
================================================
- Coverage 86.58% 85.71% -0.87%
================================================
Files 155 155
Lines 24368 24798 +430
================================================
+ Hits 21098 21255 +157
- Misses 3270 3543 +273
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas compat changes LGTM
@@ -1010,9 +1010,13 @@ def __init__( | |||
) -> None: | |||
if isinstance(path, str) and path.startswith("s3://"): | |||
self.fs_meta = {"is_s3": True, "actual_path": path} | |||
self.path = tempfile.TemporaryDirectory().name | |||
self.dir_: Optional[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do type declarations work here? Is the type only declared in one branch of the if
, or is it assumed that a type definition in one branch of the if
also applies to the other else
clause? Is it possible/better to forward-declare the type of self.dir_
before entering the if
/else
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, types in both branches must be the same. That is how mypy always works. Here's an example (I'm using sys.argv
rather than a trivial if True
because mypy is smart enough to ignore unreachable branches).
# test.py
import sys
if len(sys.argv) > 2:
x = 1
else:
x = "hello"
results in
(main) dt08% mypy test.py
test.py:5: error: Incompatible types in assignment (expression has type "str", variable has type "int") [assignment]
Note that in this example I didn't even bother annotating the type, mypy just inferred the types and noticed the incompatibility.
It is possible to forward-declare the type, but it's not something I've seen done much and not anywhere in cudf except for class members. I would be open to discussing that change, but it's out of scope for this PR since only doing it here would be inconsistent with typing everywhere else in cudf (which is already fairly inconsistent at the moment).
@@ -10,6 +10,8 @@ | |||
|
|||
import cudf | |||
|
|||
pytestmark = pytest.mark.filterwarnings("ignore::FutureWarning") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this only ignore FutureWarning
s from a particular package (or small set of packages), like cudf
or pandas
? You can specify the module name as part of this ignore syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, but I didn't see much value in it. Currently IIRC the list would be cudf, pandas, and numpy (pandas uses a deprecated numpy API somewhere in one example), but in principle it could also come from any other deprecated API that pandas calls and doesn't intercept the warning for, for instance. Given how much of our examples are copied straight from pandas, and since in examples we don't have the flexibility to insert things like warnings.simplefilter
where needed, I don't think it's worthwhile to overthink how we suppress the warnings. Dealing with those when they break seems good enough to me.
@bdice given your approval I'm going to merge to keep things moving along, but happy to continue either of the above discussions in follow-ups if you think there's a worthwhile change to be made. |
@gpucibot merge |
This PR fixes a final few warnings introduced in PRs merged after #12406 or with changes that weren't merged into #12406. More importantly, this PR updates the testing suite to always raise uncaught warnings as errors. This is a substantial change to the testing suite that will allow us to proactively react to deprecations in our dependencies, identify areas where pandas/pyarrow/some other dependency handles input data in different ways than us, etc. I've opted for the strictest possible setting, but am open to reviewer requests to be a little more lax with certain classes of warnings. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Matthew Roeschke (https://github.com/mroeschke) URL: #12468
Description
Contributes to #9999 and #10363.
When I merge these changes with #12369 I no longer see any warnings on my machine. I suspect that there will be slightly different results on different machines, so we'll see have to see how CI looks after both PRs are merged before we close #10363.
Checklist