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

gpu: mount by-path directory #1371

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Conversation

tkatila
Copy link
Contributor

@tkatila tkatila commented Apr 3, 2023

oneCCL requires the /dev/dri/by-path folder to be available to create a mapping between GPUs.

@tkatila tkatila force-pushed the gpu-by-path-inclusion branch from 57c2d80 to 3c44bba Compare April 6, 2023 07:53
@tkatila
Copy link
Contributor Author

tkatila commented Apr 6, 2023

Apparently, I hadn't added the tests for bypath.go when I created the initial commit.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2023

Codecov Report

Merging #1371 (3add9e6) into main (eeef8aa) will increase coverage by 0.32%.
The diff coverage is 100.00%.

❗ Current head 3add9e6 differs from pull request most recent head 5ac43f8. Consider uploading reports for the commit 5ac43f8 to get more accurate results

@@            Coverage Diff             @@
##             main    #1371      +/-   ##
==========================================
+ Coverage   50.56%   50.89%   +0.32%     
==========================================
  Files          44       44              
  Lines        4952     4985      +33     
==========================================
+ Hits         2504     2537      +33     
  Misses       2302     2302              
  Partials      146      146              
Impacted Files Coverage Δ
cmd/gpu_plugin/gpu_plugin.go 87.21% <100.00%> (+2.26%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@eero-t eero-t left a comment

Choose a reason for hiding this comment

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

IMHO code would be more readable if all byPath, Bypath and bypath prefixes would be changed just to path:

$ sed -i \
  -e 's/by[Pp]ath\([a-zA-Z]\)/path\1/g' \
  -e 's/By[Pp]ath\([a-zA-Z]\)/Path\1/g' \
  $(git grep -il 'bypath[a-z]')

And copyright year in the new Go files should be 2023, not 2022.

Otherwise seems fine to me.

@tkatila tkatila force-pushed the gpu-by-path-inclusion branch 3 times, most recently from 6310155 to e892112 Compare April 12, 2023 13:04
@eero-t
Copy link
Contributor

eero-t commented Apr 12, 2023

New version with nicer local variable names looks IMHO much more readable! I.e. my comments are addressed.

cmd/internal/pluginutils/bypath.go Outdated Show resolved Hide resolved
cmd/internal/pluginutils/bypath.go Outdated Show resolved Hide resolved
@tkatila tkatila force-pushed the gpu-by-path-inclusion branch from e892112 to 49c5d0a Compare April 14, 2023 12:11
mythi
mythi previously approved these changes Apr 14, 2023
Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

looks great to me, thanks!

@tkatila
Copy link
Contributor Author

tkatila commented Apr 18, 2023

Any further notes/comments?

cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved
cmd/gpu_plugin/gpu_plugin.go Outdated Show resolved Hide resolved
cmd/gpu_plugin/gpu_plugin_test.go Outdated Show resolved Hide resolved
@eero-t
Copy link
Contributor

eero-t commented Apr 19, 2023

cmd/gpu_fakedev/fakedev.go should provide fake versions of all the files that GPU plugin checks (especially if their omission would cause constant log spam / errors).

Just make sure your changes to it do not conflict with ones in Harri's PR #1369.

@tkatila
Copy link
Contributor Author

tkatila commented Apr 19, 2023

cmd/gpu_fakedev/fakedev.go should provide fake versions of all the files that GPU plugin checks (especially if their omission would cause constant log spam / errors).

I modified it to check in plugin creation for existence of the by-path dir. If it doesn't exist, it won't try to find the mounts. But if it does exist, it will warn about failures constantly.

@eero-t
Copy link
Contributor

eero-t commented Apr 19, 2023

cmd/gpu_fakedev/fakedev.go should provide fake versions of all the files that GPU plugin checks (especially if their omission would cause constant log spam / errors).

I modified it to check in plugin creation for existence of the by-path dir. If it doesn't exist, it won't try to find the mounts. But if it does exist, it will warn about failures constantly.

Ok, looks good!

Fakedev can indeed skip things that are used only by (real) workloads, and do not impact GPU plugin behavior (e.g. what resources it provides).

oneCCL requires the /dev/dri/by-path folder to be available
to create a mapping between GPUs.

Signed-off-by: Tuomas Katila <[email protected]>
@tkatila tkatila force-pushed the gpu-by-path-inclusion branch from 210ba2c to 5ac43f8 Compare April 19, 2023 12:06
@tkatila
Copy link
Contributor Author

tkatila commented Apr 19, 2023

Hopefully all comments now addressed. Please review @mythi and @uniemimu .

@uniemimu uniemimu merged commit 943e34f into intel:main Apr 20, 2023
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.

5 participants