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

Removes stale annotator exception. #4536

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

HidyHan
Copy link
Contributor

@HidyHan HidyHan commented Sep 24, 2023

Related issues

Fixes #4533.

Checklist

  • [NA] I've added a screenshot of the changes, if this is a frontend change
  • [NA] I've added and/or updated tests, if this is a backend change
  • [Y] I've run the pre-commit.sh script
  • [NA] I've updated docs, if needed

@HidyHan
Copy link
Contributor Author

HidyHan commented Sep 24, 2023

@AndrewJGaut I don't think I can directly add you as a reviewer, so tagging you here.

@AndrewJGaut
Copy link
Contributor

@HidyHan Thanks for tagging me. I went ahead and kicked off CI, but it looks like the formatting test failed. Looking closer at it, it appears the MyPy typing check failed. So perhaps the ignore comments are still needed...

Either way, you can verify by running pre-commit again locally. (It may have failed when you ran it locally, but it's a bit hard to tell what failure looks like until you've run it a few times. Otherwise, maybe it's passing locally but failing when run on GHA, which would be surprising, but I've certainly seen weirder!)

@HidyHan
Copy link
Contributor Author

HidyHan commented Oct 2, 2023

Taking a closer look.

  1. I cannot reproduce the errors locally. The mypy version is specified in requirements.dev.txt, so the version difference may be some random place upstream. FWIW my python version is 3.9 and the python version in testing environment seems to be 3.7.
  2. Our mypy requirement version was never upgraded after Add TarFile.offset python/typeshed#5210 is merged (so I am quite bewildered why I cannot reproduce the format error locally, given that my mypy version is 0.782 as required). Based on the release schedule in [1] we should upgrade to mypy 0.900+ to include the change. However, doing so triggers other errors, like this:
venv/bin/activate_this.py: error: Duplicate module named "activate_this" (also at "./codalab/lib/venv/bin/activate_this.py")
venv/bin/activate_this.py: note: Are you missing an __init__.py? Alternatively, consider using --exclude to avoid checking one of them.
Found 1 error in 1 file (errors prevented further checking)

@epicfaace any thoughts for quick fixes? If there are other benefits we can consider upgrading mypy version, but not sure whether this PR warrants it.

[1] https://mypy-lang.org/news.html

@epicfaace
Copy link
Member

Hmm @HidyHan the point of this issue was actually to upgrade mypy (#4536). See my message here (python/typeshed#5210). Basically I had contributed a missing type to mypy, and wanted to wait for a new mypy version so we can upgrade to it (so it can incorporate the new type).

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.

Remove type:ignore mypy annotations in tar_subdir_stream
3 participants