(Do not merge) 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

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.

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.

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.

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 2 times, most recently from 13c0887 to ca9cfb8 Compare December 18, 2024 05:11

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

rosbo requested changes Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(Do not merge) MVP support for Kaggle Packages #196

(Do not merge) 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

rosbo Dec 19, 2024

rosbo Dec 19, 2024

(Do not merge) MVP support for Kaggle Packages #196

Are you sure you want to change the base?

(Do not merge) 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dster2 commented Dec 18, 2024 •

edited

Loading