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

Type Check Package #1635

Open
guarin opened this issue Aug 16, 2024 · 12 comments
Open

Type Check Package #1635

guarin opened this issue Aug 16, 2024 · 12 comments

Comments

@guarin
Copy link
Contributor

guarin commented Aug 16, 2024

We would like to type check all files in the package. Adding types helps to document the code and makes it easier to use.

How to work on this issue

  1. Go to pyproject.toml to the exclude section of the mypy settings. There should be a list of files like this:
exclude = '''(?x)(
    lightly/cli/version_cli.py |
    lightly/cli/crop_cli.py |
    lightly/cli/serve_cli.py |
    lightly/cli/embed_cli.py |
    lightly/cli/lightly_cli.py |
...
  1. Pick a file you would like to type. Best are files from lightly/loss, lightly/data, lightly/utils, lightly/models/modules, and the corresponding test files. Do not type check deprecated model files in lightly/models like lightly/models/barlow_twins.py.
  2. Run mypy <filename> (make sure that you have installed lightly following the contribution guide). This should show a list with all the missing types/errors.
  3. Add types until no more errors are shown. This is a good example on how a typed file should look like: https://github.com/lightly-ai/lightly/blob/master/lightly/models/modules/memory_bank.py
  4. Go to pyproject.toml and remove the filename of the file you just typed from the mypy exclude list.
  5. If the file was the last file in a subdirectory that was missing types, then also remove the subdirectory name from the skip import list in pyprojec.toml
  6. Create a new PR named Add types for <filename> and push your changes. Make sure all Github actions pass.

Important
Lightly still supports old Python versions (including 3.7, 3.8, and 3.9) that do not work with new typing features introduced in Python 3.10. To use the new Python 3.10 syntax you have to add from __future__ import annotations at the top of the file. The following typing features changed in 3.10:

  • Union types can now be written as str | int instead of Union[str, int]
  • Optional types can now be written as str | None instead of Optional[str]
  • Tuples, lists, and dicts can now be typed as tuple[str, int], list[str], dict[str, int] instead of Tuple[str, int], List[str], Dict[str, int].

Please use from __future__ import annotations with the new types whenever possible. This will save a lot of refactoring in the future. Also feel free to update files with old types to the new syntax.

@ishaanagw
Copy link
Contributor

Hey, can I work on this issue ?

@ishaanagw
Copy link
Contributor

Hey @guarin, some of the mypy tests are failing in local, 48 errors in 23 files in master, do you think I can still push my changes for the fix of some of the files ?

@guarin
Copy link
Contributor Author

guarin commented Sep 21, 2024

Hi @ishaanagw! Thanks for contributing, feel free to work on any file :)

Hey @guarin, some of the mypy tests are failing in local, 48 errors in 23 files in master, do you think I can still push my changes for the fix of some of the files ?

Interesting, will have a look at this on Monday. Which mypy version are you using? CI uses mypy 1.4.1 with Python 3.7. The Python version should not be too important. We use 3.7 in CI to make sure we don't introduce any types that are incompatible with old Python versions.

@ishaanagw
Copy link
Contributor

Hey, @guarin Can you please review this PR - #1651

@guarin
Copy link
Contributor Author

guarin commented Sep 24, 2024

Hey @guarin, some of the mypy tests are failing in local, 48 errors in 23 files in master, do you think I can still push my changes for the fix of some of the files ?

@ishaanagw I created a PR to fix the failing mypy tests: #1654

@ishaanagw
Copy link
Contributor

Wow, that is impressive.

@guarin
Copy link
Contributor Author

guarin commented Oct 11, 2024

@ishaanagw I unassigned you from the issue to make it clearer that other people can also work on it.

@Abhrajitdas02
Copy link

Abhrajitdas02 commented Oct 12, 2024

can I work on this issue?

@Prasadayus
Copy link

can i work on this ?

@SauravMaheshkar
Copy link
Collaborator

@Prasadayus and @Abhrajitdas02 can you select some files/submodules to work on? I can create other smaller issues and then assign those to you.

@vectorvp
Copy link
Contributor

@guarin, hi! I have two questions:

  1. Where to find a list of deprecated models?
  2. What to do with mypy error Untyped decorator makes function ... untyped? Add it to ignore?

@guarin
Copy link
Contributor Author

guarin commented Nov 20, 2024

These models in lightly/models are deprecated:
Screenshot 2024-11-20 at 14 49 21

Regarding 2) you can just put a local # type: ignore[error code] comment where the error code is the reported error by mypy. See for example:

class MaskedCausalAttention(Attention): # type: ignore[misc]
"""Identical to timm.models.vision_transformer.Attention, but supports causal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants