Treat warnings as errors in tests #1182
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1182 +/- ##
==========================================
- Coverage 85.03% 82.68% -2.35%
==========================================
Files 36 36
Lines 5385 5407 +22
==========================================
- Hits 4579 4471 -108
- Misses 806 936 +130
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
.azure-pipelines.yml
Outdated
| # TODO: fix all the exceptions here | ||
| # replacing “:initializing view as actual” and “:Transforming to str index” | ||
| # with “::anndata._warnings.ImplicitModificationWarning” breaks, | ||
| # because we’re neither using editable installs nor a src/ layout | ||
| - script: > | ||
| pytest | ||
| -W error | ||
| -W 'default:initializing view as actual:UserWarning' | ||
| -W 'default:Transforming to str index:UserWarning' | ||
| -W 'default:Observation names are not unique. To make them unique:UserWarning' | ||
| -W 'default:Variable names are not unique. To make them unique:UserWarning' | ||
| -W 'default::scipy.sparse._base.SparseEfficiencyWarning' | ||
| -W 'default::dask.array.core.PerformanceWarning' |
There was a problem hiding this comment.
These are follow-up issues:
- our
ImplicitModificationWarningandnames are not uniquewarnings should be fixed in lib code and caught in test code so we can be sure none of our functions/methods invoke them, only user/test code. - scipy
SparseEfficiencyWarningand daskPerformanceWarningshould be fixed, they probably indicate real performance issues
If these were in filterwarnings and we wouldn’t have to deal with ImportPathMismatchErrors, these would be
filterwarnings = [
'error',
'default::anndata._warnings.ImplicitModificationWarning',
'default:.*names are not unique. To make them unique:UserWarning',
'default::scipy.sparse._base.SparseEfficiencyWarning',
'default::dask.array.core.PerformanceWarning',
]All of them have at least like 60 instances (sure, some are parametrized tests) and would therefore need a bit of work to be fixed.
Not in this PR.
|
only the two checkmarks in the original comment left. |
There was a problem hiding this comment.
awkward.to_categorical was deprecated in favor of awkward.str.to_categorical, which depends on pyarrow. Is making pyarrow a test dep OK or should we ignore/circuvent this?
Totally fine
write_h5ad’s as_dense failed to add IOSpec attrs
Good catch!
ExperimentalFeatureWarning
- got silenced after the first occurrence even in tests. Now it’s only silenced in non-test code after the first occurrence.
- one reason why it got raised so much in the first place is that we initialize AnnData objects in functions and methods like concat, copy, or read_*. All these places now suppress the warning.
Aren't these two points in conflict? If it was being silenced in tests, why do we see it raised so many times in the tests? Even from the same location:
Locations of ExperimentalFeatureWarning from `test_concatenate.py` on main
anndata/_core/aligned_mapping.py:64: 8 warnings
anndata/tests/test_base.py: 3 warnings
anndata/tests/test_concatenate.py: 53 warnings
anndata/tests/test_awkward.py: 39 warnings
anndata/tests/test_hdf5_backing.py: 98 warnings
anndata/tests/test_inplace_subset.py: 70 warnings
anndata/tests/test_io_elementwise.py: 10 warnings
anndata/tests/test_io_conversion.py: 17 warnings
anndata/tests/test_helpers.py: 4 warnings
anndata/tests/test_readwrite.py: 15 warnings
anndata/tests/test_io_dispatched.py: 3 warnings
anndata/tests/test_views.py: 245 warnings
anndata/tests/test_transpose.py: 13 warnings
anndata/tests/test_x.py: 7 warnings
/Users/isaac/github/anndata/anndata/_core/aligned_mapping.py:64: ExperimentalFeatureWarning: Support for Awkward Arrays is currently experimental. Behavior may change in the future. Please report any issues you may encounter!
warnings.warn(This makes me think pytest was reseting the warnings filter by itself, so we don't need to.
Also, I think we shouldn't be catching this warning so many places in the library code. These all seem like places we may want to actually throw this class of warning. I'm fine with larger changes with lots of catching in the test suite, but don't want to make large scale changes to the library code for this.
Could we add a very awkward array assignment specific filter globally to the code of the test suite, then selectively turn it off?
|
Also I think this could get a release note for any upstream deprecation warnings it fixes. |
Co-authored-by: Isaac Virshup <[email protected]>
Co-authored-by: Isaac Virshup <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Isaac Virshup <[email protected]>
|
@ivirshup ping |
Co-authored-by: Philipp A <[email protected]>
Huh, this was surprisingly low pain.
Deprecation/change fixes:
pd.Categorical.map’sna_actionparameter default was changed toNone(link). This sets it explicitly to the old default. We should probably change it toNoneand add a test.ilocforpd.Serieswhere appropriatepd.concatdoesn’t like empty DataFrames, so we filter them before passing them now: https://github.com/pandas-dev/pandas/blob/2a65fdd227734cbda5d8e33171e857ab4aaeb9d5/pandas/core/internals/concat.py#L488-L500awkward.to_categoricalwas deprecated in favor ofawkward.str.to_categorical, which depends on pyarrow. Is makingpyarrowa test dep OK or should we ignore/circuvent this?Bugfixes
write_h5ad’sas_densefailed to add IOSpec attrsExperimentalFeatureWarningAnnDataobjects in functions and methods likeconcat,copy, orread_*. All these places now suppress the warning.re_raise_errormade the traceback harder to understand, so I replaced it withadd_key_note, which doesn’t raise the error. Raising it from inside theexceptblock using plainraiseworks as expected.Deferred
adata.uns[key] = ak.Array(...)didn’t raise a warning and doesn’t do so now.