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

Colab Resolver #53

Merged
merged 2 commits into from
Jan 5, 2024
Merged

Colab Resolver #53

merged 2 commits into from
Jan 5, 2024

Conversation

mayankmalik-colab
Copy link
Collaborator

b/303240466

@rosbo
Copy link
Contributor

rosbo commented Jan 3, 2024

/gcbrun

@rosbo rosbo self-requested a review January 3, 2024 22:59
@rosbo rosbo self-assigned this Jan 3, 2024
Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Mayank! Just a few minor comments.

Can you run the linter too:

hatch run lint:fmt

https://github.com/Kaggle/kagglehub?tab=readme-ov-file#lint--format

@rosbo
Copy link
Contributor

rosbo commented Jan 4, 2024

/gcbrun

@rosbo
Copy link
Contributor

rosbo commented Jan 4, 2024

Here is the linter failure:

Step #3 - "lint": src/kagglehub/clients.py:233: error: Missing return statement  [return]
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:18: error: Signature of "is_supported" incompatible with supertype "Resolver"  [override]
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:18: note:      Superclass:
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:18: note:          def is_supported(self, handle: ModelHandle, path: str | None = ...) -> Any
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:18: note:      Subclass:
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:18: note:          def is_supported(self, h: ModelHandle, *_: Any, **__: Any) -> bool
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:31: error: Incompatible types in assignment (expression has type "int | None", target has type "str")  [assignment]
Step #3 - "lint": src/kagglehub/colab_cache_resolver.py:53: error: Incompatible types in assignment (expression has type "int | None", target has type "str")  [assignment]

@mayankmalik-colab You should be seeing the same error locally when running hatch run lint:all. Let me know if that is not the case.

@rosbo
Copy link
Contributor

rosbo commented Jan 5, 2024

/gcbrun

@rosbo
Copy link
Contributor

rosbo commented Jan 5, 2024

Yeah! All tests are passing now. Congrats @mayankmalik-colab on your first PR to kagglehub and thank you :)

@mayankmalik-colab
Copy link
Collaborator Author

Yeah! All tests are passing now. Congrats @mayankmalik-colab on your first PR to kagglehub and thank you :)

Thanks to you @rosbo . I don't know why but hatch run lint:all was not working as expected on my machine, but your pr really helped :)

@mayankmalik-colab mayankmalik-colab merged commit 2a3d3bc into Kaggle:main Jan 5, 2024
6 checks passed
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.

2 participants