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

Add Neuron backend #3018

Closed
wants to merge 4 commits into from
Closed

Add Neuron backend #3018

wants to merge 4 commits into from

Conversation

dacorvo
Copy link
Collaborator

@dacorvo dacorvo commented Feb 12, 2025

What does this PR do?

This adds the neuron backend that was previously maintained in the optimum-neuron repository.

This backend is built on top of the AWS Neuron SDK, and comprises:

  • the legacy v2 TGI launcher and router,
  • a neuron specific inference server for text-generation.

Documentation

A dedicated documentation page has been added in the backends subsection.

Tests

The backend comes with some dedicated tests:

  • neuron server tests (using only the server python package),
  • integration tests (using docker images).

I did not add continuous integration yet: waiting for advice on how to do it best.

Next steps

  • use python 3.11 and align on the main Dockerfile,
  • add a custom launcher that only exposes the relevant parameters and sets default values,
  • add a new router for servers that have a static batch size.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Comment on lines +32 to +38
test_server: install_server
python -m pip install -r ${mkfile_dir}/tests/requirements.txt
python -m pytest -sv ${mkfile_dir}/tests/server

test_integration: image
python -m pip install -r ${mkfile_dir}/tests/requirements.txt
python -m pytest -sv ${mkfile_dir}/tests/integration
Copy link
Collaborator

Choose a reason for hiding this comment

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

we may want to use uv in the future to align with the cuda backend.

this can probably be addressed later (just adding a note for reference)

@drbh
Copy link
Collaborator

drbh commented Feb 17, 2025

once we have the runner ready, I think we'll need to this to build the Dockerfile_neuron file in CI via a change similar to:

diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
index 720a13cb..ae68c4c2 100644
--- a/.github/workflows/build.yaml
+++ b/.github/workflows/build.yaml
@@ -114,6 +114,16 @@ jobs:
                 export extra_pytest="-k test_flash_gemma_simple"
                 export target=""
                 ;;
+            neuron)
+                export dockerfile="Dockerfile_neuron"
+                export label_extension="-neuron"
+                export docker_devices="none"
+                export docker_volume="/mnt/cache"
+                # export runs_on="aws-neuron-priv"
+                export platform="cpu"
+                export extra_pytest="-k test_flash_llama_simple"
+                export target=""
+                ;;
           esac
           echo $dockerfile
           echo "Dockerfile=${dockerfile}"

@drbh drbh mentioned this pull request Feb 17, 2025
The base image used to compile the rust components seems to have a low
ulimit for opened files, which leads to errors during compilation.
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.

I added you as writer to the repo so you can now push on the main repo and have a working CI.

&& 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
Copy link
Collaborator

@Narsil Narsil Feb 18, 2025

Choose a reason for hiding this comment

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

NIT: Maybe we don't download and pipe into a shell a random script (I know it's done elsewhere so not really blocking, I'm just thinking about security and reproduceability here).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will remove this in a upcoming pull-request to use python 3.11 and align Dockerfiles.


WORKDIR /usr/src

ARG CARGO_REGISTRIES_CRATES_IO_PROTOCOL=sparse
Copy link
Collaborator

Choose a reason for hiding this comment

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

That shoudn't be needed anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kept it because it is still in the main Dockerfile. Will remove it in a subsequent pull-request that aligns Dockerfiles.

COPY backends backends
COPY launcher launcher
# Remove this line once TGI has fixed the conflict
RUN cargo update ureq --precise 2.9.7
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a PR for the fix ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is obsolete: I removed it

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.

Lots of nits.

If the docker build works, maybe we should add it to the CI, no ? (So we have released versions and so on)

-v $(pwd)/data:/data \
--privileged \
-e HF_TOKEN=${HF_TOKEN} \
ghcr.io/huggingface/text-generation-inference:latest-neuron \
Copy link
Collaborator

Choose a reason for hiding this comment

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

We never show latest. Latest is causing way too many confusion.

-v $(pwd)/data:/data \
--device=/dev/neuron0 \
-e HF_TOKEN=${HF_TOKEN} \
ghcr.io/huggingface/text-generation-inference:latest-neuron \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

```
docker run -p 8080:80 \
-v $(pwd)/data:/data \
--privileged \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is --privileged really required ? Looks scary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the documentation

-e HF_TOKEN=${HF_TOKEN} \
-e HF_AUTO_CAST_TYPE="fp16" \
-e HF_NUM_CORES=2 \
ghcr.io/huggingface/text-generation-inference:latest-neuron:latest \
Copy link
Collaborator

Choose a reason for hiding this comment

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

No latest.

"backends/grpc-metadata",
"launcher",
"router"
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should probably be cleaned up at some point, this is now part of the root workspace, not a workspace itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would need to take a deeper look in an upcoming pull-request.

@dacorvo dacorvo mentioned this pull request Feb 18, 2025
@dacorvo
Copy link
Collaborator Author

dacorvo commented Feb 18, 2025

Closing this as I addressed the review comments in #3033

@dacorvo dacorvo closed this Feb 18, 2025
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