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

use venv for Tensile create on Linux #2022

Merged
merged 2 commits into from
Sep 16, 2024

Conversation

TorreZuk
Copy link
Contributor

  • if found use virtual env on Linux for Tensile create command match what has been done on Windows
  • Utilizes python updates added during initial install, e.g. rocBLAS build

@ellosel
Copy link
Contributor

ellosel commented Sep 13, 2024

@TorreZuk have you considered something like this:

https://discourse.cmake.org/t/possible-to-create-a-python-virtual-env-from-cmake-and-then-find-it-with-findpython3/1132

which is cross platform or prepending the TensileCreateLibrary command with:

source /path/to/venv/bin/activate && TensileCreateLibrary

which isn't cross platform but there must be a cross platform way of doing this.

I wasn't sure if we wanted to make these changes here or in virtualenv.cmake in rocBLAS. I personally prefer to let the client control the environment.

@TorreZuk
Copy link
Contributor Author

@ellosel I first looked at rocBLAS side but as we just call directly the cmake function TensileCreateLibraryFiles this is simpler and it still is the client who sets up the vars/hooks to enable the virtual env use. It also matches what was required and in use windows, so for now think it should suffice unless there are known user conflicts. I assume any cmake defined virtualenv (implicit function argument) would be valid for use here, otherwise it could be a new argument to TensileCreateLibraryFiles.

@TorreZuk
Copy link
Contributor Author

If this doesn't cause problems would like to merge as need to cherry pick to 6.3 branch to fix installation on new OS.

Copy link
Contributor

@ellosel ellosel left a comment

Choose a reason for hiding this comment

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

Assuming this was tested in an environment where joblib isn't available on the system and only through the venv, I am good with this change.

@TorreZuk TorreZuk merged commit 7dbd723 into ROCm:develop Sep 16, 2024
18 checks passed
@TorreZuk TorreZuk deleted the swdev-482669-fix-venv-use branch September 16, 2024 16:13
TorreZuk added a commit to TorreZuk/Tensile that referenced this pull request Sep 17, 2024
* use venv if vars set TensileCreateLibraryFiles on Linux to match Windows

(cherry picked from commit 7dbd723)
TorreZuk added a commit that referenced this pull request Sep 17, 2024
* use venv if vars set TensileCreateLibraryFiles on Linux to match Windows

(cherry picked from commit 7dbd723)
bstefanuk pushed a commit to bstefanuk/Tensile that referenced this pull request Dec 13, 2024
* use venv if vars set TensileCreateLibraryFiles on Linux to match Windows

(cherry picked from commit 7dbd723)
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.

2 participants