pymc-devs / pymc-experimental

https://pymc-experimental.readthedocs.io
Other
77 stars 49 forks source link

Handle new pymc and pytensor releases #329

Closed jessegrabowski closed 5 months ago

jessegrabowski commented 5 months ago

Closes #328

Lots of tests fail following https://github.com/pymc-devs/pymc/pull/7047, https://github.com/pymc-devs/pymc/pull/7211, and https://github.com/pymc-devs/pymc/pull/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 https://github.com/pymc-devs/pymc/pull/7227, so we should update the CI to no longer test on it.

ricardoV94 commented 5 months ago

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

323

jessegrabowski commented 5 months ago

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

jessegrabowski commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Is that PR good to merge?

On Tue, 9 Apr 2024, 08:34 Ricardo Vieira, @.***> wrote:

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

— Reply to this email directly, view it on GitHub https://github.com/pymc-devs/pymc-experimental/pull/329#issuecomment-2044245439, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACCUOSMH7ETCJZC5IM6WTY4ODWNAVCNFSM6AAAAABF5SEYNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBUGI2DKNBTHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

ricardoV94 commented 5 months ago

Is that PR good to merge?

No we still need to run on the CI

zaxtax commented 5 months ago

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

On Tue, 9 Apr 2024, 09:12 Ricardo Vieira, @.***> wrote:

Is that PR good to merge?

On Tue, 9 Apr 2024, 08:34 Ricardo Vieira, @.***> wrote:

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

— Reply to this email directly, view it on GitHub

329 (comment)

https://github.com/pymc-devs/pymc-experimental/pull/329#issuecomment-2044245439 , or unsubscribe

https://github.com/notifications/unsubscribe-auth/AAACCUOSMH7ETCJZC5IM6WTY4ODWNAVCNFSM6AAAAABF5SEYNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBUGI2DKNBTHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

No we still need to run on the CI

— Reply to this email directly, view it on GitHub https://github.com/pymc-devs/pymc-experimental/pull/329#issuecomment-2044294357, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACCUIBG4GBX4SQESS7HH3Y4OIGPAVCNFSM6AAAAABF5SEYNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBUGI4TIMZVG4 . You are receiving this because you commented.Message ID: @.***>

ricardoV94 commented 5 months ago

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

I'll probably merge it here

ricardoV94 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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

ricardoV94 commented 5 months ago

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

Already did that

ricardoV94 commented 5 months ago

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 commented 5 months ago

It's not a windows thing, I flipped which CI runs on 3.12, and now the Ubuntu fails to install

maresb commented 5 months ago

@ricardoV94, I can use conda-lock to solve on Windows, but I'm unable to reproduce any solving failures. I'm confused by your last comment, could you be more specific? (The Ubuntu CI looks like it installs but the tests fail.)

Maybe try switching to setup-micromamba as per https://github.com/pymc-devs/pymc/pull/7213?

maresb commented 5 months ago

Also, what is the defaults channel doing in the environment specs??? :joy:

ricardoV94 commented 5 months ago

@ricardoV94, I can use conda-lock to solve on Windows, but I'm unable to reproduce any solving failures. I'm confused by your last comment, could you be more specific? (The Ubuntu CI looks like it installs but the tests fail.)

At some point I tried to switch the python-version, so windows installed on python3.10 and ubuntu on 3.12, and I got the same issue on the ubuntu job then. So it's a python 3.12 issue and not an OS

ferrine commented 5 months ago

I see errors are different now

ricardoV94 commented 5 months ago

I see errors are different now

Yes, I simply removed the checks for coords/shapes to be static where they used to be.

ricardoV94 commented 5 months ago

@maresb I tried the switch to micromamba setup in the last commit. If we're lucky that fixes it.

Regarding defaults channel... no idea. Could it be needed if some packages don't exist in conda-forge?

ricardoV94 commented 5 months ago

Seems to have worked!!

jessegrabowski commented 5 months ago

What's left to do to merge this?

ricardoV94 commented 5 months ago

There is still one test failing with a FutureWarning that I can't reproduce locally

jessegrabowski commented 5 months ago

I see a numpy casting error and an error related to the Skellam distribution. I think we can just capture and ignore the casting error; it comes from casting a numpy masked_array to int. This code reproduces the warning:

np.ma.masked_array(np.array([1., 2., np.nan])).astype(int)

As for the Skellam, the computation isn't overflowing anymore. This code warns about a divide by zero but produces a valid output:

from scipy import stats
# emits warning
x1 = stats.skellam(0.9, 0.99).pmf(-3)

We could instead use the logpdf method of ncx2 directly to avoid the warning. I guess there were some issues with spurious warnings on this distribution in the past, this is maybe related? See https://github.com/scipy/scipy/pull/12244 and https://github.com/scipy/scipy/pull/5179

# no warning
x2 = 2 * np.exp(stats.ncx2.logpdf(2*0.99, 2 * (1 - (-3)), 2*0.9))
abs(x1 - x2) # 1e-17
ricardoV94 commented 5 months ago

The casting error is in one of the Marginal tests no?

jessegrabowski commented 5 months ago

Yes but the source is pymc.model.core.make_obs_var, here

ricardoV94 commented 5 months ago

For the skellam we can set np.errstate context?

ricardoV94 commented 5 months ago

This is what we do in that branch:

x = np.array([1., 2., np.nan])
np.ma.masked_array(x, np.isnan(x)).astype(int)

I guess this didn't use to trigger before?

jessegrabowski commented 5 months ago

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)
ricardoV94 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

ricardoV94 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

ricardoV94 commented 5 months ago

Maybe related to my changes to the github actions

jessegrabowski commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

ricardoV94 commented 5 months ago

Seems to have worked @lucianopaz !