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

NVIDIA Support #113

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

NVIDIA Support #113

wants to merge 5 commits into from

Conversation

Mannilie
Copy link

Changes

  • ...

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions
Copy link

Cat Gif

@@ -111,7 +111,7 @@ if [ $PLAY_MODE = true ]; then
echo "# Testing in PlayMode #"
echo "###########################"
echo ""
unity-editor \
/opt/unity/Editor/Unity \
Copy link
Member

Choose a reason for hiding this comment

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

We are using the alias on purpose - it has compatibility fixes and environment context.

Copy link
Author

Choose a reason for hiding this comment

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

I understand. But the alias is using some commands that restrict me from using nvidia-docker.

@webbertakken
Copy link
Member

Could you provide much more context about what the changes are doing exactly and why they are needed?
Also; Are these changes only needed in test-runner? Did you consider adding to the docker repository itself? What is the size of the added dependencies?

We're currently not merging anything without clear description as to why it's needed, as there are many projects using our actions and removing lines is much harder than adding them.

@Mannilie
Copy link
Author

Mannilie commented Apr 22, 2021

Appologies for the lack of detail. I've just had some really serious time constraints with work.
I actually didn't expect you to merge this fork I just thought you could use it to compare branches but nevermind.

Allow me to explain what my findings were because I think I understand what my issue was. So our projects have compute shaders and I really need the test server to perform tests on functions that utilize those computes. Otherwise, half of my test coverage would have been completely disregarded.

This is the main reason why I reached out to you guys to try to find a workaround seeing as though Unity doesn't have easy parameters for performing Compute Shader tests by force. The main objective was to get the NVidia Docker tools to work with Unity otherwise what was the point.

So here's what I found out. When running Unity headless, it requires an xhost display through Docker, otherwise Unity will automatically switch to the LLVM pipe driver (which is basically CPU drivers that simulate graphics functions) and unfortunately the biggest issue with it is it's version of OpenGL it's so outdated! To the point where Compute Shaders arent even supported.

The solution is to run the unity editor using nvidia-docker instead (which is preconfigured with all the utilities needed to run the editor headless and initialize graphics.

The index.js file was updated with the following command to get it to work with NVIDIA:
nvidia-docker run --name=super-container --security-opt seccomp=unconfined --init --net=host --privileged=true --rm=true -e DISPLAY=:0 -v /tmp/.X11-unix/X0:/tmp/.X11-unix/X0:ro -v /etc/localtime:/etc/localtime:ro -v /share-docker:/share-docker

Honestly, if it's not something that would take a huge amount of effort to be added to the main project, fair enough. Unfortunately I don't have time at the moment to integrate it cleanly into your project seeing as though it could interfere with the ecosystem you've developed in getting images into the pipeline. Do keep in mind that I'm writing all this to provide you with my findings in case someone else runs into this in the future. I'm happy forking the repository and custom-editing it for my purposes.

@webbertakken
Copy link
Member

@Mannilie Thanks a bunch for the excellent write-up of the context. It is extremely valuable to us.

It doesn't look like a huge amount of work to incorporate these changes, but we'll need to consider whether the nvidia-docker driver belongs in our scope or not. We'll look into incorporating these changes as time permits.

Copy link
Member

@webbertakken webbertakken left a comment

Choose a reason for hiding this comment

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

Just marking this as not something we should merge as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants