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

Handle new pymc and pytensor releases #329

Merged
merged 15 commits into from
Apr 16, 2024

Conversation

jessegrabowski
Copy link
Member

@jessegrabowski jessegrabowski commented Apr 8, 2024

Closes #328

Lots of tests fail following pymc-devs/pymc#7047, pymc-devs/pymc#7211, and pymc-devs/pymc#7238

It also seems that 5.13 breaks RVs with scan in the logp if they have strict=True, because the rng isn't provided. MarginalModel, Statespace, and DiscreteMarkovChain fail now. Not sure what's up with that, but it doesn't seem right. Easy fix is to set strict=False, but curious why it's happening.

The new shape rules break the r2d2 tests, @ferrine

TestSkellam has an illegal test value that results in log(0), @wd60622

Also python 3.9 is no longer being supported following pymc-devs/pymc#7227, so we should update the CI to no longer test on it.

@jessegrabowski jessegrabowski added bug Something isn't working help wanted Extra attention is needed tests labels Apr 8, 2024
@ricardoV94
Copy link
Member

ricardoV94 commented Apr 9, 2024

There was already a PR open to fix the MarginalModel. Actually the breaking changes there provide a fix to the numpyro bug.

#323

pyproject.toml Outdated Show resolved Hide resolved
@jessegrabowski
Copy link
Member Author

Check the CI run for this PR, I'm pretty sure the MarginalModel failures are new and unrelated to #323

@jessegrabowski
Copy link
Member Author

Also it seems like the windows and unbuntu runs are grabbing different versions of PyMC, because they can't agree on where dataset_to_point_list should be imported from

@ricardoV94
Copy link
Member

Also it seems like the windows and unbuntu runs are grabbing different versions of PyMC, because they can't agree on where dataset_to_point_list should be imported from

probably the python 3.9 forcing an older version of pymc

@zaxtax
Copy link
Contributor

zaxtax commented Apr 9, 2024 via email

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 9, 2024

Is that PR good to merge?

No we still need to run on the CI

@ricardoV94 ricardoV94 changed the title Fix test failure resulting from recent pymc/pytensor updates Handle new pymc and pytensor releases Apr 9, 2024
@ricardoV94 ricardoV94 added major pytensor and removed bug Something isn't working help wanted Extra attention is needed labels Apr 9, 2024
@zaxtax
Copy link
Contributor

zaxtax commented Apr 9, 2024 via email

@ricardoV94
Copy link
Member

I meant is #323 ready to not be a draft?

I'll probably merge it here

Also ignore warning from upstream dependency
@ricardoV94
Copy link
Member

ricardoV94 commented Apr 9, 2024

@maresb any idea why the Windows environment is not getting solved? https://github.com/pymc-devs/pymc-experimental/actions/runs/8614534973/job/23608187593?pr=329#step:5:568

   C:\Windows\system32\cmd.exe /D /S /C "C:\Miniconda3\condabin\mamba.bat env update --name pymc-experimental-test --file D:\a\pymc-experimental\pymc-experimental\conda-envs\setup-miniconda-patched-windows-environment-test.yml"
  Channels:
   - conda-forge
   - defaults
  Platform: win-64
  Collecting package metadata (repodata.json): ...working... done
  Solving environment: ...working... failed
  Channels:
   - conda-forge
   - defaults
  Platform: win-64
  Collecting package metadata (repodata.json): ...working... done
  Solving environment: ...working... failed
  Warning: 
  LibMambaUnsatisfiableError: Encountered problems while solving:
    - package python-3.12.2-h1d929f7_0 is excluded by strict repo priority
  
  
  
  
  LibMambaUnsatisfiableError: Encountered problems while solving:
    - package python-3.12.2-h1d929f7_0 is excluded by strict repo priority

@ricardoV94 ricardoV94 force-pushed the future-warnings branch 2 times, most recently from d01274f to b574dc3 Compare April 9, 2024 11:20
@ricardoV94
Copy link
Member

@jessegrabowski I fixed the SymbolicRandomVariables. The major change is that we require rngs to be handled manually by default now. It's just a question of specifying it in the inputs of the respective OpFromGraph constructor.

@jessegrabowski
Copy link
Member Author

I saw. I'm pushing a patch for the distributions used by statespace as well.

@ricardoV94
Copy link
Member

I saw. I'm pushing a patch for the distributions used by statespace as well.

Already did that

@ricardoV94
Copy link
Member

So we need to fix the windows CI (no idea what's going on), and the R2D2 thing. @ferrine can you look into the R2? There are no longer constant coords/dims so the code has to change

@ricardoV94
Copy link
Member

For the nan mask thing, let's add a pytest.warns for now, and open an issue on the PyMC side. We should replace nan with a discrete value after masking so that this warning isn't triggered (or suppress it)

@ricardoV94
Copy link
Member

It looks like it's set to ignore overflows, but we're getting a divide by zero and an invalid input warning. We'd have to set the errorstate to ignore both divide and invalid:

with np.errstate(divide='ignore', invalid='ignore'):
    stats.skellam(0.9, 0.99).pmf(-3)

Sounds good, we can pass a wrapper for the reference logp/logcdf that does that

@jessegrabowski
Copy link
Member Author

You want me to pass a wrapper? I just set the context on the whole test

@ricardoV94
Copy link
Member

ricardoV94 commented Apr 15, 2024

You want me to pass a wrapper? I just set the context on the whole test

That's also fine, but if it's the same work, more specific is nicer

@ricardoV94
Copy link
Member

You want me to pass a wrapper? I just set the context on the whole test

That's also fine, but if it's the same work, more specific is nicer

Ignore, I just added a comment. Looks fine

@ricardoV94
Copy link
Member

The PyPI job is failing now, that sounds like fun :)

@ricardoV94
Copy link
Member

Maybe related to my changes to the github actions

@jessegrabowski
Copy link
Member Author

jessegrabowski commented Apr 15, 2024

When I use a wrapper, the test fails completely:

        def skellam_pmf_with_errstate(value, mu1, mu2):
            with np.errstate(divide="ignore", invalid="ignore"):
               pmf_x = scipy.stats.skellam.pmf(value, mu1, mu2)
            return pmf_x

        check_logp(
            Skellam,
            I,
            {"mu1": Rplus_small, "mu2": Rplus_small},
            skellam_pmf_with_errstate,
        )

Can you reproduce? Makes me worried something else is going on inside the Op itself that is being hidden.

@ricardoV94
Copy link
Member

Reverting the actions didn't help. @lucianopaz any idea what may be going wrong?

https://github.com/pymc-devs/pymc-experimental/actions/runs/8688042500/job/23822814914?pr=329#step:5:29

ERROR: Could not install packages due to an OSError: [Errno 2] No such file or directory: '/home/runner/work/pymc-experimental/pymc-experimental/dist/pymc-experimental*.tar.gz'

@lucianopaz
Copy link
Contributor

lucianopaz commented Apr 15, 2024

Reverting the actions didn't help. @lucianopaz any idea what may be going wrong?

https://github.com/pymc-devs/pymc-experimental/actions/runs/8688042500/job/23822814914?pr=329#step:5:29

ERROR: Could not install packages due to an OSError: [Errno 2] No such file or directory: '/home/runner/work/pymc-experimental/pymc-experimental/dist/pymc-experimental*.tar.gz'

Looking at what is different between the last successful run and the ones you're getting now, it seems like the source distribution is getting built into pymc_experimental-0.0.18.tar.gz instead of pymc-experimental-0.0.18.tar.gz. I'm not sure what commit from build made this change to the output file of build sdist, but it looks like we just have to change the dash with an underline to in the PYPI workflow to get this fixed.

Co-authored-by: Ben Mares <services-git-throwaway1@tensorial.com>
@ricardoV94 ricardoV94 force-pushed the future-warnings branch 2 times, most recently from 55155d4 to 7a94946 Compare April 15, 2024 12:30
@ricardoV94
Copy link
Member

Seems to have worked @lucianopaz !

Co-authored-by: lucianopaz <luciano.paz.neuro@gmail.com>
@ricardoV94 ricardoV94 merged commit 1f70c7a into pymc-devs:main Apr 16, 2024
8 checks passed
@jessegrabowski jessegrabowski deleted the future-warnings branch April 16, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI fails after pymc-devs/pymc#7211 and pymc-devs/pymc#7238
6 participants