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

Workflow and code update for numpy versions from 1.15 to 1.21 #352

Merged
merged 27 commits into from Feb 17, 2022

Conversation

tanyuqian
Copy link
Member

@tanyuqian tanyuqian commented Feb 15, 2022

This PR fixes #333 and fixes #341

Code is updated to pass the mypy test with numpy>=1.20 (which fixes #333)

Workflow is updated to enumerate numpy versions (from 1.15 to 1.21), and further include pytorch version (1.7.1 and 1.8.1), which fixes #341

Note: In mypy.ini, I changed warn_unused_ignores to False (here) for the enumeration of numpy versions. Not sure if there are better ways to do it like in mypy-torch. Looks adding a same line under [mypy-numpy] doesn't work.

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #352 (4463ab6) into master (b83d3ec) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #352   +/-   ##
=======================================
  Coverage   80.43%   80.43%           
=======================================
  Files         135      135           
  Lines       11359    11359           
=======================================
  Hits         9137     9137           
  Misses       2222     2222           
Impacted Files Coverage Δ
texar/torch/data/embedding.py 82.25% <0.00%> (ø)
texar/torch/data/vocabulary.py 83.69% <0.00%> (ø)
texar/torch/run/metric/summary.py 92.95% <0.00%> (ø)
texar/torch/utils/dtypes.py 75.00% <ø> (ø)
texar/torch/evals/bleu_moses.py 86.76% <57.14%> (ø)
texar/torch/data/data/record_data.py 82.72% <66.66%> (ø)
texar/torch/data/data/dataset_utils.py 91.66% <100.00%> (ø)
texar/torch/evals/bleu_transformer.py 97.77% <100.00%> (ø)
texar/torch/run/metric/classification.py 89.39% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b83d3ec...4463ab6. Read the comment docs.

@hunterhector
Copy link
Member

Code is updated to pass the mypy test with numpy>=1.20

Workflow is updated to enumerate numpy versions (from 1.15 to 1.21)

Note: In mypy.ini, I changed warn_unused_ignores to False (here) for the enumeration of numpy versions. Not sure if there are better ways to do it like in mypy-torch. Looks adding a same line under [mypy-numpy] doesn't work.

Don't know why the config does not work in the my-numpy section, does @huzecong have any ideas?

@huzecong
Copy link
Collaborator

I think the [mypy-torch] section was for the PyTorch stubs files, not for code that calls PyTorch functions. I don't think there's any way to achieve the latter.

I'd imagine the type: ignore comments are added under a certain version of numpy, and does not apply to other versions? I think we can just run mypy/pylint on a single version.

@hunterhector
Copy link
Member

I see, so if I understand correctly, there are type:ignore comments in numpy and they use certain version of numpy/mypy combination that passes the tests. We will probably need to find the right version.

@huzecong
Copy link
Collaborator

I don't think mypy checks numpy code since we've set follow_imports = skip in mypy-numpy. What I was referring to is Texar code that interacts with numpy. I believe we have some type: ignore comments on lines where we call numpy functions / interact with numpy arrays, and we can do type-checking on linting only on a single numpy version.

@hunterhector
Copy link
Member

OK, now I think I understand. And I think we are doing or planning to do mypy check with only certain versions.

Now the fix would probably be, don't set warn_unused_ignores to False, fix a numpy version during mypy check, and remove unnecessary type: ignore when needed.

mypy.ini Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
hunterhector
hunterhector previously approved these changes Feb 17, 2022
@hunterhector
Copy link
Member

Thanks a lot for fixing this!

@hunterhector hunterhector dismissed their stale review February 17, 2022 06:29

find some more version related sections in code

Copy link
Member

@hunterhector hunterhector left a comment

Choose a reason for hiding this comment

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

I realize two more things

  1. Now NumPy is on 1.22, would that also work for us? For example, some of Forte check fail because of this: Texer numpy version requirement is outdated forte#634
  2. We have only updated the test workflow versions, but not the actual install version check (i.e https://github.com/asyml/texar-pytorch/blob/master/requirements.txt and https://github.com/asyml/texar-pytorch/blob/master/setup.py#L35)

@tanyuqian
Copy link
Member Author

I realize two more things

  1. Now NumPy is on 1.22, would that also work for us? For example, some of Forte check fail because of this: asyml/forte#634
  2. We have only updated the test workflow versions, but not the actual install version check (i.e https://github.com/asyml/texar-pytorch/blob/master/requirements.txt and https://github.com/asyml/texar-pytorch/blob/master/setup.py#L35)
  1. sure. numpy==1.22 for python 3.8 and 3.9 added. (numpy==1.22 requires python>=3.8)
  2. install version check updated.

@hunterhector
Copy link
Member

thanks a lot

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.

Compatibility update with pytorch>=1.7 Compatibility update with numpy>=1.20
3 participants