[runtime env] Support clone `virtualenv` from an existing `virtualenv` #22309

Catch-Bull · 2022-02-11T04:50:20Z

Why are these changes needed?

Before this PR, we can't run ray in virtualenv, cause runtime_env does not support create a new virtualenv from an existing virtualenv.

More details:#21801 (comment)

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

fyrestone · 2022-02-11T06:13:48Z

python/ray/_private/runtime_env/pip.py

+                virtualenv_path,
+            ]
+            logger.info(
+                "Creating virtualenv at %s," " current python dir %s",


Can you remove the " " in the string?

Please use f-string

Please use f-string

the reason why we use % instead of f-string in logger is in here: #21801 (comment)

fyrestone · 2022-02-11T06:15:01Z

python/ray/_private/runtime_env/pip.py

+                virtualenv_path,
+            ]
+            logger.info(
+                "Colning virtualenv %s to %s", current_python_dir, virtualenv_path


Suggested change

"Colning virtualenv %s to %s", current_python_dir, virtualenv_path

"Cloning virtualenv %s to %s", current_python_dir, virtualenv_path

thx! Fixed.

fyrestone · 2022-02-11T06:18:21Z

python/requirements.txt

@@ -84,6 +84,7 @@ pytest-rerunfailures
 pytest-sugar
 pytest-lazy-fixture
 pytest-timeout
+pytest-virtualenv
 scikit-learn==0.22.2
 tensorflow==2.5.1
 testfixtures


Can we add the virtualenv-clone to requirements.txt? @edoakes

@edoakes virtualenv-clone is just a script in only one file, I can copy it to $RAY_HOME/python/ray/_private/runtime_env/clonevirtualenv.py

https://github.com/edwardgeorge/virtualenv-clone/blob/master/clonevirtualenv.py

Can we add the virtualenv-clone to requirements.txt? @edoakes

I believe you mean is add the virtualenv-clone to ray's requirements in setup.py

better if we can copy it instead of adding another dependency

cc @pcmoritz virtualenv-clone has an MIT license -- are we ok to pull it into the repo?

architkulkarni · 2022-02-12T01:10:05Z

python/ray/_private/runtime_env/pip.py

        if cls._is_in_virtualenv():
-            # TODO(fyrestone): Handle create virtualenv from virtualenv.
+            # virtualenv-clone homepage:


Could you please add a comment about why we need virtualenv-clone in this case? It's clear from the PR description and the previous PR, but it would be good to have it as a comment in the code for easier reference.

Could you please add a comment about why we need virtualenv-clone in this case? It's clear from the PR description and the previous PR, but it would be good to have it as a comment in the code for easier reference.

virtualenv-clone README have some reasons why we shouldn't use virtualenv to relocate virtualenv:

Virtualenv provides a way to make virtualenv's relocatable which could then be copied as we wanted. However making a virtualenv relocatable this way breaks the no-site-packages isolation of the virtualenv as well as other aspects that come with relative paths and /usr/bin/env shebangs that may be undesirable. Also, the .pth and .egg-link rewriting doesn't seem to work as intended. This attempts to overcome these issues and provide a way to easily clone an existing virtualenv.

Example: If we don't rewrite .pth file in new_venv, and this file directory to old_venv sit-package's dir, then the package A which in old_venv sit-package's dir got changed, the package A of new_venv will also change accordingly.

architkulkarni · 2022-02-12T01:10:53Z

python/ray/tests/test_runtime_env_conda_and_pip_4.py

+
+    # Run current test case in virtualenv again.
+    # If the command exit code is not zero, this case will be failed.
+    cloned_virtualenv.run(f"IN_VIRTUALENV=1 python -m pytest {__file__}")


How does this make test_run_in_virtualenv fail? Do we need to assert that the exit code is nonzero somewhere?

Also, if it fails, will the test output be printed somewhere? If not, it could be hard to debug.

How does this make test_run_in_virtualenv fail? Do we need to assert that the exit code is nonzero somewhere?

Also, if it fails, will the test output be printed somewhere? If not, it could be hard to debug.

We don't need to assert exit_code != 0, this pytest-plugin will raise an exception when return_code != 0, and this Exception will construct by this pytest-cmd's stderr, so we may not worry about debug.

architkulkarni

Thanks for this follow-up! Looks good to me, once we resolve what to do about including virtual-clone as a dependency or vendoring it

Catch-Bull · 2022-02-15T16:07:01Z

python/ray/_private/runtime_env/_clonevirtualenv.py

+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+"""
+


@edoakes Can you accept I include clone-virtualenv like this?

@architkulkarni @edoakes And do you have any other comments for this PR?

Yes this looks good

I think it's okay to include it like this. According to this https://dwheeler.com/essays/floss-license-slide.html the MIT license is permissive and can be included in a project that uses the Apache license (which is what Ray uses).

No other comments from me!

ray-project#22309) Before this PR, we can't run ray in virtualenv, cause `runtime_env` does not support create a new virtualenv from an existing virtualenv. More details:ray-project#21801 (comment) Co-authored-by: 捕牛 <[email protected]>

support clone virtualenv from an existing virtualenv

b015071

Catch-Bull requested review from architkulkarni, fyrestone, edoakes and SongGuyang February 11, 2022 04:50

Catch-Bull requested review from richardliaw and ericl as code owners February 11, 2022 04:50

delete fixture workerspace

71c1901

fyrestone reviewed Feb 11, 2022

View reviewed changes

fix

f7e32bc

Catch-Bull mentioned this pull request Feb 11, 2022

[runtime env] pip-based runtime env refactor #21952

Closed

2 tasks

architkulkarni assigned architkulkarni and rkooo567 Feb 12, 2022

architkulkarni reviewed Feb 12, 2022

View reviewed changes

捕牛 added 4 commits February 14, 2022 20:09

copy clonevirtualenv.py

d8f8366

fix clone virtualenv

6123924

skip lint for clonevirtualenv

b3914f0

fix lint

9c31a79

Catch-Bull commented Feb 15, 2022

View reviewed changes

edoakes approved these changes Feb 15, 2022

View reviewed changes

edoakes merged commit 4c73560 into ray-project:master Feb 15, 2022

rkooo567 removed their assignment Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runtime env] Support clone `virtualenv` from an existing `virtualenv` #22309

[runtime env] Support clone `virtualenv` from an existing `virtualenv` #22309

Catch-Bull commented Feb 11, 2022

fyrestone Feb 11, 2022

simon-mo Feb 11, 2022

Catch-Bull Feb 11, 2022

fyrestone Feb 11, 2022

Catch-Bull Feb 11, 2022

fyrestone Feb 11, 2022 •

edited

Loading

Catch-Bull Feb 11, 2022 •

edited

Loading

Catch-Bull Feb 11, 2022 •

edited

Loading

edoakes Feb 11, 2022

edoakes Feb 11, 2022

architkulkarni Feb 12, 2022

Catch-Bull Feb 14, 2022

architkulkarni Feb 12, 2022

Catch-Bull Feb 14, 2022

architkulkarni left a comment

Catch-Bull Feb 15, 2022

edoakes Feb 15, 2022

architkulkarni Feb 15, 2022

architkulkarni Feb 15, 2022

	"Colning virtualenv %s to %s", current_python_dir, virtualenv_path
	"Cloning virtualenv %s to %s", current_python_dir, virtualenv_path

[runtime env] Support clone virtualenv from an existing virtualenv #22309

[runtime env] Support clone virtualenv from an existing virtualenv #22309

Conversation

Catch-Bull commented Feb 11, 2022

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fyrestone Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Catch-Bull Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Catch-Bull Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[runtime env] Support clone `virtualenv` from an existing `virtualenv` #22309

[runtime env] Support clone `virtualenv` from an existing `virtualenv` #22309

fyrestone Feb 11, 2022 •

edited

Loading

Catch-Bull Feb 11, 2022 •

edited

Loading

Catch-Bull Feb 11, 2022 •

edited

Loading