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

Private hf_cache fix #663

Merged
merged 7 commits into from
Sep 15, 2023
Merged

Private hf_cache fix #663

merged 7 commits into from
Sep 15, 2023

Conversation

varunshenoy
Copy link
Contributor

@varunshenoy varunshenoy commented Sep 15, 2023

Around a week ago, we noticed that builds around private Hugging Face repos started to break due to not being able to find the access token in the build.

There are two fixes that this PR introduces:

  1. Bring back the --mount directive in the dockerfile for caching. This ensures that the cache warmer can pull the secret from an external mount. To support tgi/vllm/etc., propagate the use_hf_secret variable in the Dockerfile. This will add the --mount flag when we need a secrets mount.
  2. Rename hf_access_token to hf-access-token to match with our backend settings.
  3. To ensure being able to test Trusses with tokens written in plaintext within the config.yml: pass hf_access_token to the Dockerfile and programatically add it as an env variable if it is set. This is insecure in the context of a build (in a build we read from a secrets file) but is fine for testing.

TODO:

  • add a test to ensure that the --mount is not dropped from the dockerfile

Copy link
Collaborator

@squidarth squidarth left a comment

Choose a reason for hiding this comment

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

lgtm, modulo a couple comments. awesome work tracking this down, really subtle issue. Would be awesome in the future if we could better enable sharing constants across the two repos.

@@ -17,6 +17,6 @@ RUN pip install -r /app/cache_requirements.txt --no-cache-dir && rm -rf /root/.c
COPY ./cache_warmer.py /cache_warmer.py
{% for repo, hf_dir in models.items() %}
{% for file in hf_dir.files %}
RUN python3 /cache_warmer.py {{file}} {{repo}} {% if hf_dir.revision != None %}{{hf_dir.revision}}{% endif %}
{{ "RUN --mount=type=secret,id=hf-access-token,dst=/etc/secrets/hf-access-token" if use_hf_secret else "RUN" }} python3 /cache_warmer.py {{file}} {{repo}} {% if hf_dir.revision != None %}{{hf_dir.revision}}{% endif %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you pull hf-access-token into a constant, and then pass it in via templating? We can't share constants w/ Baseten easily, but it still might make this more manageable as a constant

@varunshenoy varunshenoy merged commit cba6a3b into main Sep 15, 2023
10 checks passed
@varunshenoy varunshenoy deleted the varun/private_hf_cache_fix branch September 15, 2023 16:38
@@ -243,3 +244,20 @@ def test_truss_server_caching_truss():
)
time.sleep(15)
assert "Downloading model.safetensors:" not in container.logs()


def test_hf_cache_dockerfile():
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be marked as an integration test?

aspctu pushed a commit that referenced this pull request Sep 15, 2023
* Always upgrade when pip installing during patch (#659)

* Always upgrade when pip installing.

* Bump pyproject.

* Private `hf_cache` fix (#663)

* add fix

* bump pyproj

* update cache to read secret from correct path

* clean up pyproject.toml

* add test to check for mount

* template out hf token file name

* Bump version to 0.7.7

---------

Co-authored-by: Sidharth Shanker <sid.shanker@baseten.co>
Co-authored-by: Varun Shenoy <vnshenoy@stanford.edu>
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