Kaggle / kagglehub

Python library to access Kaggle resources
Apache License 2.0
47 stars 9 forks source link

[b/317131904] Make license_name param optional for model_upload method #62

Closed lucyhe closed 7 months ago

lucyhe commented 8 months ago

Related to: https://github.com/Kaggle/kaggleazure/pull/27324, and https://github.com/Kaggle/kaggleazure/pull/27323

Enabling this will allow other libraries (e.g. Keras) to publish private models to Model Hub (e.g. during development). Users can then set a license in the UI before making those public.

Testing

rosbo commented 8 months ago

Did you test e2e? I am getting an internal error:

$ hatch run python -c "import kagglehub; kagglehub.model_upload('rosebv/test-model/pax/kagglehub', '/usr/local/google/home/rosbo/Documents/test-dot-model-instance')"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models.py", line 53, in model_upload
    create_model_instance_or_version(h, tokens, license_name, version_notes)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models_helpers.py", line 52, in create_model_instance_or_version
    _create_model_instance(model_handle, files, license_name)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models_helpers.py", line 28, in _create_model_instance
    api_client.post(f"/models/{model_handle.owner}/{model_handle.model}/create/instance", data)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/clients.py", line 79, in post
    process_post_response(response_dict)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/exceptions.py", line 72, in process_post_response
    raise BackendError(error_message)
kagglehub.exceptions.BackendError: InternalServerError

I suspect the issue is in models_helper#_create_model_instance. It sends a None license and the backend doesn't like this. We should not include the field at all.

Also, we should fix the backend to return an InvalidArgument exception if the license passed is None.

lucyhe commented 8 months ago

Did you test e2e? I am getting an internal error:

$ hatch run python -c "import kagglehub; kagglehub.model_upload('rosebv/test-model/pax/kagglehub', '/usr/local/google/home/rosbo/Documents/test-dot-model-instance')"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models.py", line 53, in model_upload
    create_model_instance_or_version(h, tokens, license_name, version_notes)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models_helpers.py", line 52, in create_model_instance_or_version
    _create_model_instance(model_handle, files, license_name)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models_helpers.py", line 28, in _create_model_instance
    api_client.post(f"/models/{model_handle.owner}/{model_handle.model}/create/instance", data)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/clients.py", line 79, in post
    process_post_response(response_dict)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/exceptions.py", line 72, in process_post_response
    raise BackendError(error_message)
kagglehub.exceptions.BackendError: InternalServerError

I suspect the issue is in models_helper#_create_model_instance. It sends a None license and the backend doesn't like this. We should not include the field at all.

Also, we should fix the backend to return an InvalidArgument exception if the license passed is None.

Thank you! Will iterate on this!

lucyhe commented 8 months ago

I can remove license from the frontend call!

My original thought was that the backend should accept None as a value (just as it will need to handle if licenseName is missing entirely), and process it correctly, i.e. use the right default. This made me realize that I think what I missed was in my original PR, i.e. I handled None appropriately in the internal CreateModelInstanceHandler.cs but not ModelApiService/V1/CreateModelInstanceHandler.cs

lucyhe commented 7 months ago

Did you test e2e? I am getting an internal error:

$ hatch run python -c "import kagglehub; kagglehub.model_upload('rosebv/test-model/pax/kagglehub', '/usr/local/google/home/rosbo/Documents/test-dot-model-instance')"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models.py", line 53, in model_upload
    create_model_instance_or_version(h, tokens, license_name, version_notes)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models_helpers.py", line 52, in create_model_instance_or_version
    _create_model_instance(model_handle, files, license_name)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/models_helpers.py", line 28, in _create_model_instance
    api_client.post(f"/models/{model_handle.owner}/{model_handle.model}/create/instance", data)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/clients.py", line 79, in post
    process_post_response(response_dict)
  File "/usr/local/google/home/rosbo/git/kagglehub/src/kagglehub/exceptions.py", line 72, in process_post_response
    raise BackendError(error_message)
kagglehub.exceptions.BackendError: InternalServerError

I suspect the issue is in models_helper#_create_model_instance. It sends a None license and the backend doesn't like this. We should not include the field at all.

Also, we should fix the backend to return an InvalidArgument exception if the license passed is None.

The "None" input is getting validated by this compiled proto code:

    public string LicenseName {
      get { return licenseName_; }
      set {
        licenseName_ = pb::ProtoPreconditions.CheckNotNull(value, "value");
      }
    }

which returns an InternalServerError -- is that a default I can change? Aside from updating this exception type, this is ready for another look, along with https://github.com/Kaggle/kaggleazure/pull/27462, which is also required for this PRs changes to work. Thanks!

rosbo commented 7 months ago

About the InternalServerError thrown when the enum value is wrong.

I created a thread about it for another use case: https://chat.kaggle.net/kaggle/pl/p7iwokecb7g4bgur78aeethjce

Erdal will tackle it here: https://b.corp.google.com/issues/320724526