Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 19, 2024

Conversation

lucyhe
Copy link
Contributor

@lucyhe lucyhe commented Jan 12, 2024

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

@lucyhe lucyhe marked this pull request as draft January 12, 2024 00:55
@lucyhe lucyhe marked this pull request as ready for review January 12, 2024 17:21
@lucyhe lucyhe requested a review from rosbo January 12, 2024 17:21
@rosbo
Copy link
Contributor

rosbo commented Jan 12, 2024

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
Copy link
Contributor Author

lucyhe commented Jan 12, 2024

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
Copy link
Contributor Author

lucyhe commented Jan 12, 2024

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
Copy link
Contributor Author

lucyhe commented Jan 17, 2024

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
Copy link
Contributor

rosbo commented Jan 17, 2024

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

Copy link
Contributor

@rosbo rosbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's wait to merge until the necessary Kaggle webtier change is merged.

@lucyhe lucyhe merged commit 8d15344 into main Jan 19, 2024
7 checks passed
@lucyhe lucyhe deleted the lh/license branch January 19, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants