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

Fix VI test not skipping in Windows #7136

Closed
wants to merge 2 commits into from

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Feb 5, 2024

Description

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

try:
with warn_ctxt:
trace = inference.fit(n=0).sample(10000)
except NotImplementedInference as e:
pytest.skip(str(e))

It should come from

if _known_scan_ignored_inputs([self.approx.model.logp()]):
raise NotImplementedInference(
"SVGD does not currently support Minibatch or Simulator RV"
)

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

  • Closes #
  • Related to #

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

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

@ricardoV94 ricardoV94 added bug tests VI Variational Inference Github CI/CD no releasenotes Skipped in automatic release notes generation labels Feb 5, 2024
Copy link

codecov bot commented Feb 5, 2024

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

@@             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 Coverage Δ
pymc/variational/operators.py 55.31% <0.00%> (-1.21%) ⬇️

... and 58 files with indirect coverage changes

@ricardoV94 ricardoV94 changed the title Fix VI failing test Fix VI test not skipping in Windows Feb 5, 2024
@ricardoV94 ricardoV94 added the winOS windows OS related label Feb 5, 2024
@aerubanov
Copy link
Contributor

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

@ricardoV94
Copy link
Member Author

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

@aerubanov
Copy link
Contributor

Yeah, will try to find something relevant for this issue

@aerubanov
Copy link
Contributor

aerubanov commented Feb 6, 2024

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: pytest-dev/pytest#11129

@ricardoV94
Copy link
Member Author

Great, how should we fix it though :D?

@aerubanov
Copy link
Contributor

@ricardoV94 I see two options:

  • Do not use pytest.warn() here and check for warning manualy
  • Keep list of parameters for which NotImplementedInference should be raised and call pytest.skip() before interface.fit()

@ricardoV94
Copy link
Member Author

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
Copy link
Contributor

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

@ricardoV94 ricardoV94 removed the winOS windows OS related label Feb 6, 2024
@aerubanov
Copy link
Contributor

@ricardoV94 could you please take a look #7144?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Github CI/CD help wanted no releasenotes Skipped in automatic release notes generation tests VI Variational Inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants