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 deps #1446

Merged
merged 4 commits into from
Jul 11, 2024
Merged

bump deps #1446

merged 4 commits into from
Jul 11, 2024

Conversation

miller-ian
Copy link
Contributor

@miller-ian miller-ian commented Jul 11, 2024

numpy + tox issues resolved with upgrade to numpy version 1.26.4

Tox py312 defaults to using numpy 2.0. Some numpy issues have been identified here and here. Dictating numpy version 1.26.4 (most recent version prior to 2.0) in tox.ini file resolves issues

@miller-ian miller-ian marked this pull request as draft July 11, 2024 01:54
@miller-ian miller-ian marked this pull request as ready for review July 11, 2024 02:01
@miller-ian miller-ian marked this pull request as draft July 11, 2024 02:14
@marcharper
Copy link
Member

Some of the types have changed, causing test failures like so:

  037     >>> import axelrod
  038     >>> axelrod.game.DefaultGame.RPST()
  Expected:
      (3, 1, 0, 5)
  Got:
      (np.int64(3), np.int64(1), np.int64(0), np.int64(5))

To be clear this appears to be due to numpy changes, not any of our type hints.

@miller-ian
Copy link
Contributor Author

Some of the types have changed, causing test failures like so:

  037     >>> import axelrod
  038     >>> axelrod.game.DefaultGame.RPST()
  Expected:
      (3, 1, 0, 5)
  Got:
      (np.int64(3), np.int64(1), np.int64(0), np.int64(5))

To be clear this appears to be due to numpy changes, not any of our type hints.

yep, this is an issue with tox + numpy. tox appears to default to using latest numpy version which, as you mention, gives us some problems. Reverting to numpy 1.26.4 seems to solve issue

@@ -27,6 +27,7 @@ deps =
pytest-sugar
isort
black
numpy==1.26.4
Copy link
Member

Choose a reason for hiding this comment

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

Should we pin this in pyproject.toml instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its in there as well!

Copy link
Member

Choose a reason for hiding this comment

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

In there it's not hard pinned. Just a lower bound. I wonder if it's worth hard pinning it.

Copy link
Member

Choose a reason for hiding this comment

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

We have avoided doing this in the past (I think mainly on my request?) but I wonder if it's worth starting to do (in our installation docs we do recommend using a venv etc...)...

We shouldn't delay merging this though, @miller-ian's contribution is a super helpful bug fix that lets other PRs get through :)

@drvinceknight
Copy link
Member

Thanks for this: it looks good to me.

@miller-ian miller-ian marked this pull request as ready for review July 11, 2024 12:42
@marcharper marcharper merged commit 44a8217 into Axelrod-Python:dev Jul 11, 2024
7 checks passed
@miller-ian miller-ian deleted the ian/bump-deps branch July 11, 2024 14:16
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.

3 participants