Add type annotations for mypy #746
Conversation
.gitignore
Outdated
| @@ -65,3 +65,5 @@ target/ | |||
|
|
|||
| # Rider/IntelliJ | |||
| .idea/ | |||
|
|
|||
| mypy | |||
There was a problem hiding this comment.
I looked in kagglehub and we ignore the following:
# mypy
.mypy_cache/
.dmypy.json
dmypy.json
https://github.com/Kaggle/kagglehub/blob/main/.gitignore#L141C1-L144C11
I doubt mypy writes to a file named mypy...
There was a problem hiding this comment.
I suppose I could remove that. I added it just to have a place to stash mypy stuff, like the html report and error output. I'm still running it manually, but eventually will script hatch to do it.
kaggle/api/kaggle_api_extended.py
Outdated
| else: | ||
| group = self.lookup_enum(CompetitionListTab, group) | ||
| group_val = cast(CompetitionListTab, self.lookup_enum(CompetitionListTab, group)) |
There was a problem hiding this comment.
Can lookup_enum return a type based on the type of it's first argument? Similar to generics in Java. This may solve the cast issue but I am not sure that's possible.
There was a problem hiding this comment.
I tried to make that happen but was not able to figure it out. Since then, I have seen a reference to generics in the mypy docs, so I'll try again.
There was a problem hiding this comment.
It looks like it could be made to work, if we switch to 3.12. The TypeVar syntax isn't available in prior versions. I don't think we can make that switch, but I'm going to see if I can do it anyway, as an exercise.
There was a problem hiding this comment.
I could not find a way to specify that the return type was an instance of a class-type parameter. I ended up adding an additional parameter to the method, which is a sample of the correct type. Looks like this:
request.framework = self.lookup_enum(ModelFramework, ModelFramework.MODEL_FRAMEWORK_API, framework)Not ideal, but it will do.
With a little work in the code that prompts users to upgrade, it seems like we can switch to requiring Python 3.12.
pyproject.toml
Outdated
| "idna", | ||
| "python-dateutil>=2.5.3", | ||
| "python-slugify", | ||
| # "certifi>=14.05.14", |
There was a problem hiding this comment.
Why did you need to comment these out? Is it because they are now unused?
There was a problem hiding this comment.
That's WIP. There was a lot of stuff that Swagger needed that we don't need. I haven't tested to see if the stuff I'm planning to remove is actually unused, but intend to.
| )''' | ||
| python_version = 3.12 | ||
|
|
||
| # Start off with these |
There was a problem hiding this comment.
Is the goal to eventually use the default mypy config like we do for kagglehub?
https://github.com/Kaggle/kagglehub/blob/b7317a2448f68e766e04a05718ad55823fc35a65/pyproject.toml#L91
There was a problem hiding this comment.
Yes, if I have time. The commented-out stuff comes from the mypy docs. Again, WIP.
|
Converting to Python 3.12 will require some work here to get integration tests working. |
|
The integration tests fail: I think that means we need a new docker image that uses Python 3.12. Not sure how to fix that. |
|
For now, using 3.11 syntax and all checks pass. |
I moved
FileListfrom the bottom of the file to the top.I introduced a number of typed variables to represent the return type. Not sure why, but
mypyclaimed they had typeAnyotherwise. This is mostly in the response objects, which are defined inkagglesdk, and I don't type-check that library because it has a lot of errors. That may have something to do with it.The
lookup_enum()method exists for legacy code/backward compatibility. To eliminate a bunch of casts, I added a parameter that is a sample value of the enum that needs to have a value extracted.Type checking uncovered some errors in code I haven't tested. I kinda had to fix them to move on. Likewise, I noticed a number of docstring comments that needed some work, so I did it.