Add support for pagination to lists of files #572

stevemessick · 2024-04-23T23:06:55Z

This does not include the generated code, which should make reviewing easier. Once this is merged and the final handler PR is in, I'll do a release and commit the new generated code.

jmasukawa

Nice! Overall this LG, but i'll let Jim have the final approval here.

src/KaggleSwagger.yaml

jmasukawa · 2024-04-27T01:51:34Z

src/kaggle/api/kaggle_api_extended.py

@@ -93,8 +92,9 @@ def __init__(self, fullpath, format):
    def __enter__(self):
        self._temp_dir = tempfile.mkdtemp()
        _, dir_name = os.path.split(self._fullpath)
-        self.path = shutil.make_archive(os.path.join(self._temp_dir, dir_name),
-                                        self._format, self._fullpath)
+        self.path = shutil.make_archive(


[no action] just curious, what environment did you develop in? was this kaggle-web-dev container?

going forward, we should make sure we're all using the same workflow. Mod had used his mac (developed locally) so that's why the formatting diffs are different here.

if you've used kaggle-web-dev, that's totally fine, and we'll just make sure to keep using that (at least until we have a public docker container for dev for this project).

the whitespace difference is from differing yapf versions

Yes, I'm running it in the container. I pretty much have to if I want to work on the C# and Python together. I tried opening the project in IntelliJ (because the Python plugin for Rider is currently not operational), but quickly realized I'd have to go through and install all the dependencies again. I didn't want to do that, since I don't know how many are pre-installed in the container. I mention that because I thought that's where the reformatting occurred (but yapf is more likely). I did restore almost all the format changes.

I agree that running from the container has the best chance of making sure anyone (inside Kaggle) can make a release. Can this be documented somewhere?

In the Development section, I'm adding this text. Let me know if there's any problem with it:

Kaggle Internal

Obviously, this depends on Kaggle services. When you're extending the API and modifying
or adding to those services, you should be working in your Kaggle mid-tier development
environment. You'll run Kaggle locally, in the container, and test the Python code by
running it in the container so it can connect to your local testing environment.

That's the Development section of README.md.

In case you are not aware, the releases for kagglehub and kaggle-api are done using Google Cloud Build. Anyone can make a release in a consistent environment. You just need to trigger Cloud Build with a CLI command.

Docmentation for kagglehub: https://g3doc.corp.google.com/cloud/kaggle/models/g3doc/kagglehub.md#step-2-create-a-new-release

For kaggle-api, it is documented here: https://g3doc.corp.google.com/company/teams/kaggle/cli/index.md#release

src/kaggle/api/kaggle_api_extended.py

jmasukawa · 2024-04-27T01:58:24Z

src/kaggle/api/kaggle_api_extended.py

@@ -2664,21 +2759,29 @@ def model_initialize(self, folder):
            raise ValueError('Invalid folder: ' + folder)

        meta_data = {
-            'ownerSlug': 'INSERT_OWNER_SLUG_HERE',


[no action on this PR] we may want to figure out how to use the latest version of yapf for formatting, sooner rather than later. the one in kaggle-web-dev is stuck on an older version because of that specific linux OS version.

IMHO, the other syntax here "before" (using later yapf) is much nicer.

src/kaggle/api/kaggle_api_extended.py

jplotts

Looking good! Can you please revert the license removals? Also, I'm a bit unsure about the models API w.r.t. framework/instance/versions being required or not (specific questions below). Thanks for tackling this!

jplotts · 2024-04-29T16:03:33Z

src/KaggleSwagger.yaml

+          name: pageSize
+          type: integer
+          default: 1
+          description: Page size


tiny nit - Some say "Page size" and others "Number of items per page (default 20)". Can this be consistent for all entities?

Changed to "Number of items per page (default 20)".

jplotts · 2024-04-29T16:09:51Z

tests/test_commands.sh

+kaggle kernels files hermengardo/ps4e4-ensemble-eda --page-size=5 # valid page token required
+kaggle datasets files nelgiriyewithana/apple-quality --page-size=7 --page-token=abcd


nit - consider using Kaggle-owned data for tests. For datasets, kaggle/meta-kaggle seems like a good option, and possibly use one of our learn notebooks or lastplacelarry for kernels.

It seems kaggle/meta-kaggle is not available when running on localhost, and lastplacelarry doesn't have any kernels on localhost. But I found some alternates.

jplotts · 2024-04-29T17:19:15Z

docs/KaggleApi.md

 [**models_create_instance**](KaggleApi.md#models_create_instance) | **POST** /models/{ownerSlug}/{modelSlug}/create/instance | Create a new model instance
 [**models_create_instance_version**](KaggleApi.md#models_create_instance_version) | **POST** /models/{ownerSlug}/{modelSlug}/{framework}/{instanceSlug}/create/version | Create a new model instance version
 [**models_create_new**](KaggleApi.md#models_create_new) | **POST** /models/create/new | Create a new model
 [**models_list**](KaggleApi.md#models_list) | **GET** /models/list | Lists models
+[**models_list_files**](KaggleApi.md#models_list_files) | **GET** /models/list/{ownerSlug}/{modelSlug} | List model files


What does this list if there are multiple instances / variations? (Not important to respond to this comment, but can it be documented somewhere?)

I think this is incorrect and I'm not sure why it works. The CLI does not permit framework and instance to be omitted. I'm changing it to allow the version number to be optional, though. (MT PR is linked below.)

jplotts · 2024-04-29T17:21:26Z

kaggle/__init__.py

@@ -1,19 +1,3 @@
-#!/usr/bin/python


Is this intentionally removed? I would think we'd want to keep it.

It was removed as part of file generation, not intentionally.

The installation of /tmp/autogen.sh failed when running in the container. I added instructions to the readme to work around that problem.

jplotts · 2024-04-29T17:27:23Z

docs/KaggleApi.md

 [**metadata_get**](KaggleApi.md#metadata_get) | **GET** /datasets/metadata/{ownerSlug}/{datasetSlug} | Get the metadata for a dataset
 [**metadata_post**](KaggleApi.md#metadata_post) | **POST** /datasets/metadata/{ownerSlug}/{datasetSlug} | Update the metadata for a dataset
 [**model_instance_versions_download**](KaggleApi.md#model_instance_versions_download) | **GET** /models/{ownerSlug}/{modelSlug}/{framework}/{instanceSlug}/{versionNumber}/download | Download model instance version files
+[**model_instance_versions_files**](KaggleApi.md#model_instance_versions_files) | **GET** /models/{ownerSlug}/{modelSlug}/{framework}/{instanceSlug}/{versionNumber}/files | List model instance version files


Is the versionNumber required? Would it be possible for it to be optional (like for datasets, kernels, etc) and to assume the latest if it's not provided?

https://github.com/Kaggle/kaggleazure/pull/29404 adds support for using the latest version if none is specified.

jplotts · 2024-04-29T17:29:39Z

src/kaggle/api/kaggle_api_extended.py

@@ -93,8 +92,9 @@ def __init__(self, fullpath, format):
    def __enter__(self):
        self._temp_dir = tempfile.mkdtemp()
        _, dir_name = os.path.split(self._fullpath)
-        self.path = shutil.make_archive(os.path.join(self._temp_dir, dir_name),
-                                        self._format, self._fullpath)
+        self.path = shutil.make_archive(


I agree that running from the container has the best chance of making sure anyone (inside Kaggle) can make a release. Can this be documented somewhere?

Add support for pagination to lists of files

ddda962

stevemessick marked this pull request as draft April 23, 2024 23:07

stevemessick added 4 commits April 23, 2024 23:08

cleanup

7517e49

Page competition file lists

68d7c6d

Restore original format

64d0cf1

Restore lost items

ae22113

stevemessick marked this pull request as ready for review April 26, 2024 21:40

stevemessick requested a review from jmasukawa April 26, 2024 21:41

jmasukawa requested a review from jplotts April 27, 2024 02:04

jmasukawa reviewed Apr 27, 2024

View reviewed changes

Review comments

b0ca209

jplotts reviewed Apr 29, 2024

View reviewed changes

stevemessick added 2 commits April 29, 2024 22:24

Restore licenses

9aed8a1

Address review comments; add work-around for autogen.sh failure

5e4664f

stevemessick requested a review from jplotts May 1, 2024 20:20

jplotts approved these changes May 2, 2024

View reviewed changes

stevemessick merged commit be3906e into main May 2, 2024
4 checks passed

stevemessick deleted the page-file-lists branch May 2, 2024 14:47

batubb mentioned this pull request Jun 4, 2024

competition_list_files returns incomplete list of competition files #511

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pagination to lists of files #572

Add support for pagination to lists of files #572

stevemessick commented Apr 23, 2024 •

edited

Loading

jmasukawa left a comment

jmasukawa Apr 27, 2024

stevemessick Apr 29, 2024

jplotts Apr 29, 2024

stevemessick Apr 29, 2024

stevemessick Apr 29, 2024

rosbo Apr 30, 2024

jmasukawa Apr 27, 2024

jplotts left a comment

jplotts Apr 29, 2024

stevemessick May 1, 2024

jplotts Apr 29, 2024

stevemessick May 1, 2024

jplotts Apr 29, 2024

stevemessick May 1, 2024 •

edited

Loading

jplotts Apr 29, 2024

stevemessick May 1, 2024

stevemessick May 1, 2024

jplotts Apr 29, 2024

stevemessick May 1, 2024

jplotts Apr 29, 2024

		kaggle kernels files hermengardo/ps4e4-ensemble-eda --page-size=5 # valid page token required
		kaggle datasets files nelgiriyewithana/apple-quality --page-size=7 --page-token=abcd

Add support for pagination to lists of files #572

Add support for pagination to lists of files #572

Conversation

stevemessick commented Apr 23, 2024 • edited Loading

jmasukawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kaggle Internal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplotts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemessick May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemessick commented Apr 23, 2024 •

edited

Loading

stevemessick May 1, 2024 •

edited

Loading