search

asyml / texar-pytorch

Integrating the Best of TF into PyTorch, for Machine Learning, Natural Language Processing, and Text Generation. This is part of the CASL project: http://casl-project.ai/
https://asyml.io
Apache License 2.0
745 stars 117 forks source link

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

Closed tanyuqian closed 2 years ago

tanyuqian commented 2 years ago

This PR fixes https://github.com/asyml/texar-pytorch/issues/333 and fixes https://github.com/asyml/texar-pytorch/issues/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[bot] commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

hunterhector commented 2 years ago

Thanks a lot for fixing this!

tanyuqian commented 2 years ago

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 commented 2 years ago

thanks a lot