Conversation
dolaameng
left a comment
There was a problem hiding this comment.
LGTM. Thank you @nl917 !
Would like @stevemessick to take a look.
| @@ -1,4 +1,4 @@ | |||
| __version__ = "0.1.16" | |||
| __version__ = "0.1.17" | |||
There was a problem hiding this comment.
I don't know the release process of kaggle-sdk but I suppose there is some manual step here to actually publish this package. I don't know how kaggle-cli imports this package. It used be a manual copy but I am not sure if that has changed. @stevemessick knows better
There was a problem hiding this comment.
Just realized Steve is OOO this week. Requested review from Jonathan instead
There was a problem hiding this comment.
Yes @nl917 I think these are experimental features only with placeholder code but not real implementation? Checking in the code is to help unblock downstream application. But if this got published to pypi, we probably just wait till everything is implemented by @andrewmwang first.
There was a problem hiding this comment.
I'm also not super familiar with recent implementations/updates to the sdk but I can do a quick look over and see if anything stands out
There was a problem hiding this comment.
Once this is merged and a new version of kagglesdk is published to pypi (docs here: https://g3doc.corp.google.com/cloud/kaggle/apis/g3doc/kagglesdk.md#how-to-push-to-pypi), the way Kaggle CLI gets the new package is by bumping it's dependency here: https://github.com/Kaggle/kaggle-cli/blob/main/pyproject.toml#L27
it is currently set to 0.1.16 and we'd change that to 0.1.17.
then we'd need to do a release of Kaggle CLI so our users can pick up the new version. we can test that Kaggle CLI deps pull in the new kagglesdk version locally by:
hatch env remove default
hatch run kaggle --help
(that 2nd line could be any command. we just want to call something to rebuild the CLI)
it'd be good to run the Kaggle CLI tests too to make sure everything is working: https://github.com/kaggle/kaggle-cli?tab=readme-ov-file#tests
btw this assumes in your local CLI env you didn't do anything like pip install kagglesdk --break-system-packages (b/c then it would pull from that global install vs. try to install based on the .toml definition)
| The slug for the task without the user's namespace. (ex: my-task instead of | ||
| username/my-task). This should match the slug in the kaggle-benchmarks | ||
| decorator in the code. | ||
| text (str) |
There was a problem hiding this comment.
@andrewmwang Can you confirm this the text for the .py source code? And on the server side, a .py file with delimiters like # %% will be converted to a ipython notebook properly?
| r""" | ||
| Attributes: | ||
| task_slugs (ApiBenchmarkTaskSlug) | ||
| model_slugs (str) |
There was a problem hiding this comment.
@nl917 @andrewmwang what's the behavior here when model_slugs is empty list []. Will it run on all models, or no models?
There was a problem hiding this comment.
should we make it so that it runs on the default model? that might be the most common option
There was a problem hiding this comment.
should we make it so that it runs on the default model? that might be the most common option
From a client perspective, the -m here works like a model filter, so if nothing specified, it's probably more intuitive to default to run ALL models. Wdyt?
There was a problem hiding this comment.
My concern is that the model list will only keep increasing. And right now we can only do five runs at a times. will it be too laggy if we run it on all models? I feel like we don't want to make it too easy to do this.
There was a problem hiding this comment.
Yes that's a good point. In that case can go back to default model.
stevemessick
left a comment
There was a problem hiding this comment.
RSLGTM -- I don't usually read the code from kapigen.
jmasukawa
left a comment
There was a problem hiding this comment.
Thanks for waiting for my datasets changes. LGTM!
…ds (#958) Leverages changes to `DatasetApiService/V1/UpdateDatasetMetadataHandler` and `DatasetApiService/V1/GetDatasetMetadataHandler` to accept expected update frequency and user specified sources. Wait before merging: - [x] MT changes are deployed: Kaggle/kaggleazure#41349 - [x] Correct set of kagglesdk changes are made / merged and release is pushed: Kaggle/kaggle-sdk-python#31 Other changes: - Pin `virtualenv` version so `./docker-hatch` works - Run lint / format before making any changes (there was drift) - Docs updates for datasets / datasets metadata Local testing: - [screencast](http://go/scrcast/NTI4NTQxMDQ2ODAwMzg0MHwwNjg0MjViNC1iNQ) cc: @goeffthomas http://b/500108129
Uh oh!
There was an error while loading. Please reload this page.