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

Bump Aesara to 2.7.5, aeppl to 0.0.32, update tests for aeppl #5955

Merged
merged 5 commits into from
Jul 7, 2022

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jul 6, 2022

What is this PR about?

This is in preparation for a new version number of PyMC in order to delineate a point in time after the pymc-base Conda-Forge package was produced.

NOTE: This relies on the not-yet-existent v0.0.32 release of aeppl.

For Aesara and aeppl I bumped the version numbers in conda-envs/*.yml and requirements*.txt.

For PyMC I bumped the version number in pymc/__init__.py.

Checklist

Major / Breaking Changes

  • ...

Bugfixes / New features

  • ...

Docs / Maintenance

  • Bump dependencies Aesara to 2.7.5, aeppl to 0.0.32

@maresb maresb changed the title Bump PyMC to 4.1.2, Aesara to 2.7.5, aeppl to 0.0.32 Bump Aesara to 2.7.5, aeppl to 0.0.32 Jul 6, 2022
@maresb maresb closed this Jul 6, 2022
@maresb maresb reopened this Jul 6, 2022
@maresb
Copy link
Contributor Author

maresb commented Jul 6, 2022

Waiting on conda-forge/aeppl-feedstock#35 or autotick-bot

@maresb maresb closed this Jul 6, 2022
@maresb maresb reopened this Jul 6, 2022
@maresb maresb marked this pull request as ready for review July 6, 2022 22:51
@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #5955 (5a0f7b1) into main (7fe5164) will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5955      +/-   ##
==========================================
- Coverage   89.54%   89.42%   -0.13%     
==========================================
  Files          73       73              
  Lines       13273    13292      +19     
==========================================
+ Hits        11885    11886       +1     
- Misses       1388     1406      +18     
Impacted Files Coverage Δ
pymc/distributions/logprob.py 97.72% <ø> (ø)
pymc/__init__.py 68.42% <0.00%> (-31.58%) ⬇️

@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2022

The test expects a KeyError to be raised, but the behavior was modified in aesara-devs/aeppl#151.

@twiecki
Copy link
Member

twiecki commented Jul 7, 2022

@maresb Want to fix that here? Sounds like a simple one.

@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2022

@twiecki I'm stuck because I'm confused what is the intention of the relevant test. I haven't ever used aeppl before, so I'm unfortunately missing context.

In the first joint_logp call, aeppl v0.0.31 raised a KeyError, but this changed via aesara-devs/aeppl#151 in v0.0.32. Now both the first and second calls to joint_logp return Sum{acc_dtype=float64}.0. Thus from the perspective of the current test, ignore_logprob is having no effect.

I was wondering if it made sense in the first case to add warn_missing_rvs=True as a kwarg, but that results in got multiple values for keyword argument here.

Any insight into what is the desired behavior here? Ping @ricardoV94

@ricardoV94
Copy link
Member

@twiecki I'm stuck because I'm confused what is the intention of the relevant test. I haven't ever used aeppl before, so I'm unfortunately missing context.

In the first joint_logp call, aeppl v0.0.31 raised a KeyError, but this changed via aesara-devs/aeppl#151 in v0.0.32. Now both the first and second calls to joint_logp return Sum{acc_dtype=float64}.0. Thus from the perspective of the current test, ignore_logprob is having no effect.

I was wondering if it made sense in the first case to add warn_missing_rvs=True as a kwarg, but that results in got multiple values for keyword argument here.

Any insight into what is the desired behavior here? Ping @ricardoV94

We were relying on the fact that aeppl would fail with ignore_missing_rvs=False. Now we should revert it to True and test that it issues a warning in the first case but not the second.

@@ -5,8 +5,8 @@ channels:
- defaults
dependencies:
# Base dependencies
- aeppl=0.0.31
- aesara=2.7.4
- aeppl=0.0.32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some other file, either pre-commit or mypy something that specifies aesara/aeppl versions that have to be updated as well.

Checking the last commit where these dependencies were changed should show it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Last time we actually missed bumping Aesara in this location.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to squash this last commit with my previous "Bump Aesara" and "Bump aeppl" commits?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just squash-merge once we get greens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need, if you don't mind we squashing all the commits when merging. Otherwise you can.

We are less strict about this here than in Aesara for example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squash merge sounds great! 😅

@maresb maresb changed the title Bump Aesara to 2.7.5, aeppl to 0.0.32 Bump Aesara to 2.7.5, aeppl to 0.0.32, update tests for aeppl Jul 7, 2022
@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2022

All green! Ready to squash merge? 🚀

@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2022

As soon as we get a release on PyPI I can generate the new Conda-Forge build, and then as long as there are no unexpected issues, this whole Conda recipe revamp will be complete. 😄

@ricardoV94 ricardoV94 merged commit 966aabf into pymc-devs:main Jul 7, 2022
@maresb maresb deleted the bump-versions branch July 7, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants