Skip to content

feat: add support for HF_HUB_USER_AGENT_ORIGIN to add user-agent Origin field in Hub requests. #3061

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

Merged
merged 3 commits into from
Mar 4, 2025

Conversation

Hugoch
Copy link
Member

@Hugoch Hugoch commented Feb 26, 2025

What does this PR do?

Adds support for HF_HUB_USER_AGENT_ORIGIN to add user-agent Origin field in Hub requests.
Useful to track collabs.

@Hugoch Hugoch requested review from Narsil and mfuntowicz February 28, 2025 14:54
@Hugoch Hugoch marked this pull request as ready for review February 28, 2025 14:54
@@ -18,7 +18,7 @@ RUN apt-get update -y \
&& rm -rf /var/lib/apt/lists/* \
&& apt-get clean

RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --default-toolchain 1.80.1 --profile minimal -y
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- --default-toolchain 1.84.0 --profile minimal -y
Copy link
Member

Choose a reason for hiding this comment

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

You can use rust-toolchain.toml:

COPY rust-toolchain.toml rust-toolchain.toml
RUN curl -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path --default-toolchain none

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also not pipe anything into sh while we're at it, but there are unfortunately a lot of these occurrences.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure of what would be the best way to avoid the pipe. rustup-init checks the architecture to download the right installer which we need to do. We could copy rustup-init in the repo and add a check for rust-init signature for each architecture but needs to be updated for each update. Do you have other ideas in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's already alright to checksum the installer (although the installer of the installer should probably have the same treatment.).

There's the simple "we need deterministic builds" aspect. And then there's the "we don't want supply chain attacks" side of things. checksum goes a long way.

Ideal is a key word here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use rust-toolchain.toml:

COPY rust-toolchain.toml rust-toolchain.toml
RUN curl -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path --default-toolchain none

This command does not install any toolchain, which was precisely the whole point of the previous command. Am i missing something ?

Copy link
Member

@angt angt Mar 5, 2025

Choose a reason for hiding this comment

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

Yes but the correct toolchain (from rust-toolchain.toml) will be installed with cargo-chef.
At least it works here:

COPY rust-toolchain.toml rust-toolchain.toml
RUN curl -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path --default-toolchain none
ENV PATH="/root/.cargo/bin:$PATH"
RUN cargo install cargo-chef --locked

Copy link
Collaborator

@dacorvo dacorvo Mar 5, 2025

Choose a reason for hiding this comment

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

I still don't understand how it could work: the code snippet in llamacpp is different than the one you suggested above for neuron.
This one installs explicitly the 1.85.0 toolchain, instead of specifying none, which doesn't install any toolchain.

Copy link
Member

Choose a reason for hiding this comment

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

It was working, the correct toolchain was automatically installed at this step:

RUN cargo install cargo-chef --locked 

But I confirm it is not the case anymore.

Narsil
Narsil previously approved these changes Feb 28, 2025
Copy link
Collaborator

@Narsil Narsil left a comment

Choose a reason for hiding this comment

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

LGTM

@Narsil Narsil merged commit d8ff7f2 into main Mar 4, 2025
28 checks passed
@Narsil Narsil deleted the feat/add-custom-origin-headers branch March 4, 2025 15:43
@dacorvo
Copy link
Collaborator

dacorvo commented Mar 5, 2025

This pull-request breaks the neuron build: was it tested ?

@Hugoch
Copy link
Member Author

Hugoch commented Mar 5, 2025

@dacorvo Yes, it builds on https://github.com/huggingface/text-generation-inference/actions/runs/13627605158/job/38160431550
Seems that main now refers to 1.85.0 which fails

@dacorvo
Copy link
Collaborator

dacorvo commented Mar 5, 2025

@dacorvo Yes, it builds on https://github.com/huggingface/text-generation-inference/actions/runs/13627605158/job/38160431550 Seems that main now refers to 1.85.0 which fails

The issue is not related to 1.85.0: the build you are mentioning was before the review.
Here is the failing build with the changes introduced after the review: https://github.com/huggingface/text-generation-inference/actions/runs/13657446044/job/38180115060

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.

4 participants