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] Add the latest numpy version to requirements.txt #731

Merged
merged 4 commits into from
Jun 26, 2021
Merged

Conversation

anj-s
Copy link
Contributor

@anj-s anj-s commented Jun 25, 2021

What does this PR do?

Certain PRs (looks like forks) are running into mypy errors due to a bumped up numpy version that is being pulled in.

  • Bumped up numpy to >=1.21 as a temporary fix. We ideally don't want to have any other dependency other than PyTorch.
  • Since np.append is untyped to begin with I did not see any fix other than adding an ignore. Maybe I am missing something?
  • I am still debugging how we are pulling in numpy 1.21 for PRs that are opened outside of the fairscale repo. PRs within the repo are using numpy 1.20. I tried invalidating the cache but that did not make a difference. PRs on different forks don't share a cache with the repo according to circleci. Maybe we want to consider that in the future.

Before submitting

  • Did you have fun?
    • Make sure you had fun coding 🙃
  • Did you read the contributor guideline?
  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
    • N/A
  • Did you make sure to update the docs?
    • N/A
  • Did you write any new necessary tests?
    • N/A
  • Did you update the changelog? (if needed)
    • N/A

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 25, 2021
@anj-s anj-s marked this pull request as draft June 25, 2021 22:10
Copy link
Contributor

@min-xu-ai min-xu-ai left a comment

Choose a reason for hiding this comment

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

LGTM

@anj-s anj-s merged commit 00ec9ff into master Jun 26, 2021
@anj-s anj-s deleted the fix-mypy-typing branch June 26, 2021 00:59
@omry
Copy link

omry commented Jun 28, 2021

This is conflicting with fairseq on Python 3.6:
https://github.com/pytorch/fairseq/blob/master/setup.py#L196-L197

I am not sure what is the reason for the restriction there, but requiring 1.21 is breaking fairseq right now.

@omry
Copy link

omry commented Jun 28, 2021

More info:
numpy dropped Python 3.6 support in version 1.20.
As of now, fairseq still supports Python 3.6.

This diff makes Python 3.7 a hard requirement for fairscale, and thus for fairseq which depends on it.

Since the motivation here seems to be typing, I don't think it's worth changing the minimal supported Python version for fairseq and fairscale for it.

@min-xu-ai
Copy link
Contributor

Thanks for the heads up, @omry! How about we add the requirement to requirements-test.txt? That should make our CI happy without require users to have newer numpy. @anj-s, what do you think?

@omry
Copy link

omry commented Jun 28, 2021

I get an uneasy feeling thinking that the CI will test on versions other than the official minimal versions.

@anj-s
Copy link
Contributor Author

anj-s commented Jun 28, 2021

Thank you for the headsup @omry As you can see in the PR description I am still debugging why some users ran into this. This was only a quick fix to unblock users.
@min-xu-ai your suggestion sounds good. Opened a PR to make this change since we don't want to break fairseq.

@anj-s
Copy link
Contributor Author

anj-s commented Jun 28, 2021

Merged PR. @omry Let me know if this unblocks you. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants