pymc-devs / pymc

Bayesian Modeling and Probabilistic Programming in Python
https://docs.pymc.io/
Other
8.62k stars 1.99k forks source link

Fix VI test not skipping in Windows #7136

Closed ricardoV94 closed 7 months ago

ricardoV94 commented 7 months ago

Description

The test test_fit_start[SVGD-mini] stopped raising a NotImplementedInferenceError in Windows recently.

https://github.com/pymc-devs/pymc/blob/f8b19a52c19dcc77aaabd6dc7464bee9304ebc58/tests/variational/test_inference.py#L206-L210

It should come from https://github.com/pymc-devs/pymc/blob/2051d0b422a10ef0477cff0b3994ede4ec7c26ad/pymc/variational/operators.py#L145-L148

It still works correctly on Linux?

Link to failing test: https://github.com/pymc-devs/pymc/actions/runs/7783900052/job/21223348962?pr=7136

Related Issue

Checklist

Type of change


📚 Documentation preview 📚: https://pymc--7136.org.readthedocs.build/en/7136/

codecov[bot] commented 7 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6554683) 52.44% compared to head (aef772d) 67.58%. Report is 1 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pymc-devs/pymc/pull/7136/graphs/tree.svg?width=650&height=150&src=pr&token=JFuXtOJ4Cb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)](https://app.codecov.io/gh/pymc-devs/pymc/pull/7136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) ```diff @@ Coverage Diff @@ ## main #7136 +/- ## =========================================== + Coverage 52.44% 67.58% +15.13% =========================================== Files 85 101 +16 Lines 16441 16956 +515 =========================================== + Hits 8623 11460 +2837 + Misses 7818 5496 -2322 ``` | [Files](https://app.codecov.io/gh/pymc-devs/pymc/pull/7136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | Coverage Δ | | |---|---|---| | [pymc/variational/operators.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy92YXJpYXRpb25hbC9vcGVyYXRvcnMucHk=) | `55.31% <0.00%> (-1.21%)` | :arrow_down: | ... and [58 files with indirect coverage changes](https://app.codecov.io/gh/pymc-devs/pymc/pull/7136/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)
aerubanov commented 7 months ago

@ricardoV94 I try to run this test locally on Linux. It is fails when I use pytest==8.0and skipped with pytest==7.*

ricardoV94 commented 7 months ago

Thanks @aerubanov that's some nice info. Could you dig anything that changed with pytest 8.0?

aerubanov commented 7 months ago

Yeah, will try to find something relevant for this issue

aerubanov commented 7 months ago

Looks like in pytest 7 missing warning were ignored if some exception was raised, but pytest 8 checking them and that`s why this test fails. Relevant PR: https://github.com/pytest-dev/pytest/pull/11129

ricardoV94 commented 7 months ago

Great, how should we fix it though :D?

aerubanov commented 7 months ago

@ricardoV94 I see two options:

ricardoV94 commented 7 months ago

First seems slightly better? Do you want to push on this PR? If for some reason you don't have permissions you can also just open a new one

aerubanov commented 7 months ago

Yeah, looks like first will require less changes in the future. Will try to implement it.

aerubanov commented 7 months ago

@ricardoV94 could you please take a look https://github.com/pymc-devs/pymc/pull/7144?