MVP support for Kaggle Packages #196

dster2 · 2024-12-18T03:08:51Z

Support tracking accessed datasources and writing / reading them to a requirements.yaml file.
Support a "Package Scope" using that file which applies those datasource versions at runtime if the user requests a datasource without an explicit version. Kaggle Packages' __init__.py uses these helpers to automatically apply the package scope to all methods defined in the package.
Support importing a Package. This uses a handle equivalent to a Notebook handle, and like Notebooks is currently limited to the latest version, with version support coming soon.
Support Package Asset files whose path honors the current Package Scope.

Not sure who should review this, so casting a wide net of potentially interested parties 😅 Please include others who might be interested

Current plan is to not merge into main quite yet, we'll retain a workaround to use this new kagglehub version for beta Package testing, and rollout to main and our Docker image in January.

http://b/377958580

neshdev · 2024-12-18T03:16:59Z

src/kagglehub/packages.py

+        apply(module)
+
+
+logger = logging.getLogger(__name__)


nit: can you move this up?

Done, thanks

neshdev · 2024-12-18T03:18:29Z

src/kagglehub/packages.py

+                if getattr(member, "__module__", None) != module.__name__:
+                    continue
+                # These denylisted entries cause infinite loops
+                elif inspect.isclass(member) and name not in ["__base__", "__class__"]:


What is the worst case depth of this? Can we use an explicit stack here?

This only recurses on classes defined within the user's module, so you'd have to have a ton of nested classes to hit the limit, but fair point to be defensive and it removes one of the helper functions here, fixed.

neshdev · 2024-12-18T03:22:27Z

src/kagglehub/packages.py

+        self.path: pathlib.Path = package_path
+        self.datasources: VersionedDatasources = read_requirements(package_path / "kagglehub_requirements.yaml")
+
+        self._token_stack = deque()


This can just be a list.

Nice, thanks!

neshdev · 2024-12-18T03:34:22Z

src/kagglehub/packages.py

+    Otherwise, assumes we're in an interactive Kaggle Notebook where package
+    data should be written to the /kaggle/working dir."""
+    scope = PackageScope.get()
+    package_path = scope.path if scope else pathlib.Path("/kaggle/working/package")


What if we are on colab or offline?

Any time a Package is imported -- whether via kagglehub.package_import or directly, whether on Colab or offline -- the package's code should be applying its own PackageScope (through logic in the generated __init__.py file, with the helpers above in this file) so we assume it's only unset when you're defining the Package, currently intended only within a Kaggle Notebook, though we'll probably want to add more flexibility in the future.

Added a bit more docstring on this point, thanks.

neshdev · 2024-12-18T03:36:42Z

@dster2 - When ready, please add unit test for all the source files as well.

dster2 · 2024-12-18T04:43:45Z

Thanks Nesh! Yes will work on unit tests, just trying to jump-start review on the core patterns + impl

rosbo

The general direction makes sense. I added a few suggestion to make the code cleaner and less error-prone.

rosbo · 2024-12-19T18:10:04Z

src/kagglehub/handle.py

@@ -34,6 +34,11 @@ class ModelHandle(ResourceHandle):
    def is_versioned(self) -> bool:
        return self.version is not None and self.version > 0

+    def with_version(self, version: int):  # noqa: ANN201


instead of using noqa to ignore the warning, can you simply define the return type?

def with_version(self, version: int) -> ModelHandle:

Unfortunately not, you can't refer to your defining class inside yourself :\ there is typing.Self for this purpose, but it didn't work with Python 3.9 I think. There was another workaround that was also a bit messy so I thought to just denylist it... can take another pass though.

rosbo · 2024-12-19T18:10:17Z

src/kagglehub/handle.py

@@ -57,6 +62,9 @@ class DatasetHandle(ResourceHandle):
    def is_versioned(self) -> bool:
        return self.version is not None and self.version > 0

+    def with_version(self, version: int):  # noqa: ANN201


Same, please specify the DatasetHandle return type.

rosbo · 2024-12-19T18:10:37Z

src/kagglehub/handle.py

@@ -15,15 +15,15 @@
 NUM_UNVERSIONED_NOTEBOOK_PARTS = 2  # e.g.: <owner>/<notebook>


-@dataclass
+@dataclass(frozen=True)


was frozen=True suggested automatically by the linter?

No, this is required to make it a hashable type to use as key in the VersionedDatasources dictionary.

rosbo · 2024-12-19T18:29:03Z

src/kagglehub/http_resolver.py

@@ -45,6 +47,7 @@ def __call__(

        if not api_client.has_credentials():
            if cached_path:
+                register_accessed_datasource(h, None)


It would be cleaner to use a Python decorator: https://realpython.com/primer-on-python-decorators/#adding-syntactic-sugar

This way, you could simply annotate the call method of each resolver.

For the unversioned handle case, we mutate the handle to include the version so you could just pick that up when running logic post running the decorator wrapped method.

I wrote a quick POC: https://admin.kaggle.com/code/rosebv/register-access-decorator?scriptVersionId=213907476 (shared with Kaggle admins :))

https://screenshot.googleplex.com/vWyJ3M7AwC2utMU

Yeah, this is great, thanks! Definitely cleans up the cruft where I was calling this before each return statement. However it's not quite so easy, at least with the current setup... since I made the handles immutable (frozen=True) I think the decorator cannot see a modified handle from its perspective of just seeing the args. Will play around more to see if we can make it work though.

I discussed a bit offline with Vincent before his leave, this is a great idea but not as trivial as one might hope. We brainstormed some alternatives still using decorators, in the end I've gone with refactoring Resolver to use abstract _resolve() implemented by subclasses, with base __call__ wrapping it and calling register_accessed_datasource. This coincides with a change to the Resolver return value from str (path) to tuple[str, Optional[int]] (path + version) which Package support requires, though of course open to alternatives in both cases.

rosbo · 2024-12-19T18:30:32Z

src/kagglehub/http_resolver.py

@@ -278,6 +294,11 @@ def _extract_archive(archive_path: str, out_path: str) -> None:


 def _get_current_version(api_client: KaggleApiV1Client, h: ResourceHandle) -> int:
+    # Check if there's a Package in scope which has stored a version number used when it was created.
+    version_from_package = get_package_datasource_version_number(h)


nit: I think version_from_package_scope would be clearer.

rosbo · 2024-12-19T18:30:58Z

src/kagglehub/kaggle_cache_resolver.py

@@ -83,7 +84,9 @@ def __call__(
                    f"You can acces the other files othe attached competition at '{cached_path}'"
                )
                raise ValueError(msg)
+            register_accessed_datasource(h, None)


We can use a similar decorator pattern.

Support tracking accessed datasources and writing / reading them to a requirements.yaml file. Support a "Package Scope" using that file which applies those datasource versions at runtime if the user requests a datasource without an explicit version. Support importing a Package. This uses a handle equivalent to a Notebook handle, and like Notebooks is currently limited to the latest version, with version support coming soon. Support Package Asset files whose path honors the current Package Scope.

dster2 · 2025-01-28T05:50:11Z

This is more or less ready for MVP, with the notable exception of Dependency Manager support which should come soon and can be a follow-up PR.

neshdev

LGTM. I think the changes could have been introduced more iteratively. ex: interface change, refactoring, new feature.

neshdev · 2024-12-18T03:53:47Z

src/kagglehub/requirements.py

@dster2 - IIUC, we have a yaml file, and it should conform to some schema at some specified version. We then parse the contents. After, we do some type of transformation on it.

I think this could be extended to json-schema for validation and Pydantic for deserialization.
@rosbo - Im not sure if we need another library, but this custom deserialization and transformation is a repeated pattern in our code base. Do you think we should have a canonical way to deserialize directly to a class.

@neshdev Thanks, agreed we could be more structured here. Just to be clear, are you suggesting we look into that now before launch, or just a potential investment for future versions? Obviously with the latter we'd have to continue to support the current patterns, which should be easy enough by checking the specified version as you noted, but does add some cruft.

This is for future effort.

neshdev · 2025-01-29T22:03:47Z

src/kagglehub/requirements.py

Can you rename this to requirements_generator.py? I can see it getting confused with the requirements.txt file in classic packages.

Hmm, well generator also has quite another meaning in Python :) but it's a great point, I will think of something.

neshdev · 2025-01-29T22:18:39Z

src/kagglehub/resolver.py

-    def __call__(self, handle: T, path: Optional[str] = None, *, force_download: Optional[bool] = False) -> str:
+    def __call__(
+        self, handle: T, path: Optional[str] = None, *, force_download: Optional[bool] = False
+    ) -> tuple[str, Optional[int]]:
        """Resolves a handle into a path with the requested model files.


nit: the docs seem out of date.

neshdev · 2025-01-29T22:19:15Z

src/kagglehub/resolver.py

+    ) -> tuple[str, Optional[int]]:
+        """Resolves a handle into a path with the requested model files.
+
+        Args:


nit: see above. Docs are out of date.

dster2 requested review from rosbo, djherbis, KeijiBranshi and jplotts December 18, 2024 03:08

neshdev reviewed Dec 18, 2024

View reviewed changes

dster2 force-pushed the packages branch 3 times, most recently from 31b4c18 to 0d1a162 Compare December 18, 2024 04:42

dster2 force-pushed the packages branch 3 times, most recently from ca9cfb8 to f6bf2d9 Compare December 19, 2024 15:17

rosbo requested changes Dec 19, 2024

View reviewed changes

dster2 force-pushed the packages branch from f6bf2d9 to 10782d2 Compare January 28, 2025 05:38

dster2 requested a review from neshdev January 28, 2025 05:45

dster2 changed the title ~~(Do not merge) MVP support for Kaggle Packages~~ MVP support for Kaggle Packages Jan 28, 2025

neshdev approved these changes Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVP support for Kaggle Packages #196

MVP support for Kaggle Packages #196

dster2 commented Dec 18, 2024 •

edited

Loading

neshdev Dec 18, 2024

dster2 Dec 18, 2024

neshdev Dec 18, 2024

dster2 Dec 18, 2024

neshdev Dec 18, 2024

dster2 Dec 18, 2024

neshdev Dec 18, 2024

dster2 Dec 18, 2024

neshdev commented Dec 18, 2024

dster2 commented Dec 18, 2024

rosbo left a comment

rosbo Dec 19, 2024

dster2 Dec 19, 2024

rosbo Dec 19, 2024

rosbo Dec 19, 2024

dster2 Dec 19, 2024

rosbo Dec 19, 2024

dster2 Dec 19, 2024

dster2 Jan 28, 2025 •

edited

Loading

rosbo Dec 19, 2024

rosbo Dec 19, 2024

dster2 commented Jan 28, 2025

neshdev left a comment

neshdev Dec 18, 2024

dster2 Jan 30, 2025

neshdev Jan 30, 2025

neshdev Jan 29, 2025

dster2 Jan 30, 2025

neshdev Jan 29, 2025

neshdev Jan 29, 2025

MVP support for Kaggle Packages #196

Are you sure you want to change the base?

MVP support for Kaggle Packages #196

Conversation

dster2 commented Dec 18, 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

Choose a reason for hiding this comment

neshdev commented Dec 18, 2024

dster2 commented Dec 18, 2024

rosbo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dster2 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dster2 commented Jan 28, 2025

neshdev 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dster2 commented Dec 18, 2024 •

edited

Loading

dster2 Jan 28, 2025 •

edited

Loading