-
Notifications
You must be signed in to change notification settings - Fork 67
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
refactor: address linting suggestions #127
Conversation
CandiedCode
commented
Apr 6, 2024
•
edited
Loading
edited
- remove unused imports
- remove unnecessary f-strings
- remove use of in with literals
- add doc string and type hinting support to pytorch_sentiment_analysis notebook
- use magic command for %pip to make sure packages are installed within the current kernel.
- use lazy concatenation for loggers
As of Python 3.8, using is and is not with constant literals will produce a SyntaxWarning.
2d1543c
to
6d3c559
Compare
This is due to the reportOverlappingOverload from pylance. See python/mypy#14772 for more information. This also adds __future__ annotations to add support to use | since modelscan supports a min of python 3.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes looks good on a quick glance. Do you mind updating the notebook without pushing the new output? We would like to keep the original demos intact.
@iamfaisalkhan I reverted the output of the notebook. Would it make sense to have docker support to make it possible to have reproducible notebook runs, that way it's not tied to a users specific setup? If you are open, I can do a devcontainer, which github also supports, setup or we can just have a simple dockerfile as an alternative seperate from this pr. What do you think @iamfaisalkhan? |
That's a good idea. Open to either devcontainer or Docker file in a separate PR. The These changes look good to me. |