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

chore: Made it easier to use the latest framework version when calling Model.upload_* #997

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Feb 3, 2022

Goal: make it easier for the calling functions to ask for the latest framework version.
It turns out that the current approach complicates calling the upload_* functions, because there is no values of the *_version parameter that mean "latest version". This leads to the need to have different code paths depending on whether the user has given an explicit version value.
Example of the problem:

def my_import_xgboost_model(
    model_path: str,
    xgboost_version: Optional[str] = None,
):
    if xgboost_version:
            # Case for when the user has provided explicit version
            aiplatfor.Model.upload_xgboost_model(model_path=model_path, xgboost_version=xgboost_version)
    else:
            # Case for when the user has not provided explicit version
            aiplatfor.Model.upload_xgboost_model(model_path=model_path)

With this small change, the call is simplified:

def my_import_xgboost_model(
    model_path: str,
    xgboost_version: Optional[str] = None,
):
    aiplatfor.Model.upload_xgboost_model(model_path=model_path, xgboost_version=xgboost_version)

P.S. It would be great if the get_prebuilt_prediction_container_uri supported the "latest version" feature.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

…lling Model.upload_*

Goal: make it easier for the calling functions to ask for the latest framework version.
It turns out that the current approach complicates calling the upload_* functions, because there is no values of the *_version parameter that mean "latest version".
This leads to the need to have different code paths depending on whether the user has given an explicit version value.
Example of the problem:

```python
def my_import_xgboost_model(
    model_path: str,
    xgboost_version: Optional[str] = None,
):
    if xgboost_version:
            aiplatfor.Model.upload_xgboost_model(model_path=model_path, xgboost_version=xgboost_version)
    else:
            aiplatfor.Model.upload_xgboost_model(model_path=model_path)
```

With this small change, the call is simplified:

```python
def my_import_xgboost_model(
    model_path: str,
    xgboost_version: Optional[str] = None,
):
    aiplatfor.Model.upload_xgboost_model(model_path=model_path, xgboost_version=xgboost_version)
```

P.S. It would be great if the `get_prebuilt_prediction_container_uri` function supported the "latest version" feature.
@Ark-kun Ark-kun force-pushed the chore--Made-it-easier-to-request-the-latest-framework-version-when-calling-Model.upload_- branch from a40b730 to e69b58e Compare February 3, 2022 04:18
@sasha-gitg sasha-gitg merged commit 11d9af3 into googleapis:main Feb 3, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
Ark-kun added a commit to Ark-kun/pipeline_components that referenced this pull request Aug 1, 2022
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.

None yet

2 participants