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

Patching for generativeai Python package #1329

Merged
merged 22 commits into from
Dec 3, 2023
Merged

Patching for generativeai Python package #1329

merged 22 commits into from
Dec 3, 2023

Conversation

psbang
Copy link
Contributor

@psbang psbang commented Nov 29, 2023

A patch for the generativeai Python package that makes sure all requests are directed through our data proxy and configurations are set accordingly.

http://b/308644984

Copy link
Contributor

@Philmod Philmod left a comment

Choose a reason for hiding this comment

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

Please write unit tests to make sure the new package and your patch work

patches/generativeaipatch.py Outdated Show resolved Hide resolved
@psbang psbang requested a review from rosbo November 30, 2023 19:20
Dockerfile.tmpl Outdated
@@ -647,6 +651,10 @@ ADD patches/sitecustomize.py /root/.local/lib/python3.10/site-packages/sitecusto
# Override default imagemagick policies
ADD patches/imagemagick-policy.xml /etc/ImageMagick-6/policy.xml

# Add generativeai Python patch
ADD patches/generativeaipatch.py /root/.local/lib/python3.10/site-packages/generativeaipatch.py
CMD ["python", "/root/.local/lib/python3.10/site-packages/generativeaipatch.py"]
Copy link
Contributor

Choose a reason for hiding this comment

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

CMD is the Dockerfile instruction to update the command that is run when you start a container with the image. Do not update the CMD.

You can add logic that needs to be called to sitecustomize.py which gets call when a notebook session starts up.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g.

if not hasattr(sys, 'frozen'):
sys.meta_path.insert(0, GcpModuleFinder())

patches/generativeaipatch.py Outdated Show resolved Hide resolved
patches/generativeaipatch.py Outdated Show resolved Hide resolved
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.

Add a test that calls a fake HTTP client and assert that the headers are set properly.

Example test with a test HTTP server: https://github.com/Kaggle/kagglehub/blob/66f2121adccae70f05c72594113a7e463fa6ddce/tests/test_http_model_download.py#L19

@@ -73,3 +75,27 @@ def exec_module(self, module):

if not hasattr(sys, 'frozen'):
sys.meta_path.insert(0, GcpModuleFinder())

@wrapt.when_imported('google.generativeai')
Copy link
Contributor

@rosbo rosbo Nov 30, 2023

Choose a reason for hiding this comment

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

Is this called only the first time this module is imported or again every time?

To confirm, you can simply add a print statement and test in a notebook by running "import 'google.generativeai" a few times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only called the first time it seems.

@rosbo
Copy link
Contributor

rosbo commented Nov 30, 2023

Before merging, make sure to also test in the kernels editor using the psbang-genaipatch-pretest image tag like we discussed in our 1:1 earlier this week :)

patches/sitecustomize.py Outdated Show resolved Hide resolved
default_metadata = kwargs['default_metadata']
else:
default_metadata = []
kwargs['transport'] = 'rest' # Only support REST requests for now
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this is hard coded here, how are you going to enable grpc when it's ready? All the users using this image will have this code forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I'll set an environment variable then.

client_options = kwargs['client_options']
else:
client_options = {}
client_options['api_endpoint'] = os.environ['KAGGLE_DATA_PROXY_URL'] + '/palmapi'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same: hard coding this endpoint here means you will never be able to change it without breaking all the users using this image

@Philmod
Copy link
Contributor

Philmod commented Dec 1, 2023

Please also confirm that installing another version of the package doesn't break your import patch:

!pip install ... ==another-version

import ...
# patch still works

@Philmod
Copy link
Contributor

Philmod commented Dec 2, 2023

Please write unit tests to make sure the new package and your patch work

I'm guessing you were able to test this image and it worked as expected?

@Philmod
Copy link
Contributor

Philmod commented Dec 3, 2023

@psbang I'm merging this PR for you to get ready for a release tomorrow morning.

@Philmod Philmod merged commit 3fdce73 into main Dec 3, 2023
3 checks passed
@Philmod Philmod deleted the psbang-genaipatch branch December 3, 2023 18:40
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.

3 participants