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(runner): remove duplicate pynvml dep #319

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rickstaa
Copy link
Member

@rickstaa rickstaa commented Dec 9, 2024

This pull request removes the duplicate pynvml dependency.

@rickstaa rickstaa force-pushed the remove_duplicate_deps branch from fa47156 to 0b1fb07 Compare December 9, 2024 13:02
This commit removes the duplicate pynvml dependency.
@rickstaa rickstaa force-pushed the remove_duplicate_deps branch from 0b1fb07 to 2ff0f77 Compare December 9, 2024 13:02
@@ -35,14 +35,14 @@ RUN cd /comfyui/custom_nodes && \
cd ComfyUI-Misc-Effects && \
git checkout c6b360c78611134c3723388170475eb4898ff6b7

RUN pip install torch==2.5.1 torchvision torchaudio tqdm nvidia-ml-py==12.560.30
Copy link
Member Author

Choose a reason for hiding this comment

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

@leszko The only scenario where hardcoding this in the Dockerfile seems necessary is when a dependency chain installs an incorrect version of nvidia-ml-py before the requirements.txt file is processed. If this is the case we should remove it from the requirements so that we don't have two definitions that can conflict. Can you check that it works without hardcoding it in the docker?

@rickstaa
Copy link
Member Author

rickstaa commented Dec 9, 2024

@victorges this was a part duplicate of #323. Maybe we can add the comment so engineers know that the library is already included under a different name.

@victorges
Copy link
Member

@rickstaa hmmm that's super weird then, do you have an idea why were we getting errors when importing pynvml on some images like liveportrait?

@mjh1
Copy link
Contributor

mjh1 commented Dec 9, 2024

I get the same error with streamdiffusion too (running on tensordoc) e.g. docker run livepeer/ai-runner:live-app-streamdiffusion-675f99a
Screenshot 2024-12-09 at 16 55 19

@leszko
Copy link
Contributor

leszko commented Dec 10, 2024

I get the same error with streamdiffusion too (running on tensordoc) e.g. docker run livepeer/ai-runner:live-app-streamdiffusion-675f99a Screenshot 2024-12-09 at 16 55 19

Yeah, then I suggest not merging this PR, because it will break our environments.

We need to research and fix these dependencies, but it's not the highest priority right now

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

As stated by @mjh1 this breaks the streamdiffusion + TRT runtime.

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