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

Add typing to the codebase #887

Closed
hadim opened this issue Feb 17, 2020 · 11 comments · Fixed by #912
Closed

Add typing to the codebase #887

hadim opened this issue Feb 17, 2020 · 11 comments · Fixed by #912
Assignees
Labels
docs Documentation related feature Is an improvement or enhancement good first issue Good for newcomers let's do it! approved to implement

Comments

@hadim
Copy link
Contributor

hadim commented Feb 17, 2020

Python typing is a good way to rapidly check for coding issue while developing and it also allows to generate documentation with types automatically.

It's a good first issue for someone who wants to contribute to PL.

@hadim hadim added feature Is an improvement or enhancement help wanted Open to be worked on labels Feb 17, 2020
@hadim hadim mentioned this issue Feb 17, 2020
@Borda Borda added docs Documentation related good first issue Good for newcomers labels Feb 18, 2020
@awaelchli
Copy link
Member

I like this and I'm happy to help out here. My suggestion is to do it gradually and split into multiple PRs, e.g., starting with Trainer and LightningModule, then Loggers and so on.

@awaelchli awaelchli mentioned this issue Feb 21, 2020
5 tasks
@Borda Borda removed the help wanted Open to be worked on label Feb 22, 2020
@Borda
Copy link
Member

Borda commented Feb 22, 2020

@awaelchli thx and happy to see more your PRs soon :]

@Borda Borda mentioned this issue Feb 26, 2020
@Borda Borda linked a pull request Feb 26, 2020 that will close this issue
5 tasks
@williamFalcon
Copy link
Contributor

awesome! let's keep this going as we keep making fixes

@Borda
Copy link
Member

Borda commented Mar 7, 2020

It is not done yet lol

@Borda Borda reopened this Mar 7, 2020
@awaelchli
Copy link
Member

@hadim I am looking at adding more typing to the rest of the code base and I am facing this issue again with failing imports (e.g. in tests). I think it is related to forward referencing / cyclic references.
For example, in the callbacks I need to import the Trainer and Lightning classes to type the callback signatures, which somehow fails at runtime.
Have you faced such issues before?

@hadim
Copy link
Contributor Author

hadim commented Mar 14, 2020

No, I haven't but an easy fix is to use types as string. Instead of:

class Node:
    def next() -> Node:
        pass

Try:

class Node:
    def next() -> 'Node':
        pass

@luiscape
Copy link
Contributor

This StackOverflow response adds more context to @hadim's response:

In short, that's an upcoming feature of Python 4.0. The most backward compatible workaround is the solution suggested by @hadim above.

@awaelchli
Copy link
Member

Thanks for your suggestions. With just the string method there are no errors, but my IDE (pycharm) is complaining and suggesting that I still need to import Trainer (when I declare trainer: 'Trainer'). However, when I add this import and run tests, it fails to import.
The stackoverflow post doesn't say if the import is needed or not. Probably it does, otherwise how would the type system know which class it is?

@Borda
Copy link
Member

Borda commented Mar 14, 2020

Wow, is there road map for Python 4.0? What will be the last 3.x series? @luiscape

@stale
Copy link

stale bot commented May 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label May 13, 2020
@Borda
Copy link
Member

Borda commented May 13, 2020

I guess we can close it as we are continuously adding with every new PR...

@Borda Borda closed this as completed May 13, 2020
@Borda Borda added let's do it! approved to implement and removed won't fix This will not be worked on labels May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related feature Is an improvement or enhancement good first issue Good for newcomers let's do it! approved to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants