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

Enable Installation of Python Package with System lib in a Virtual Environment #9349

Merged
merged 4 commits into from
Jul 4, 2023

Conversation

oliverholworthy
Copy link
Contributor

@oliverholworthy oliverholworthy commented Jun 30, 2023

Goal

Enable Installation of Python Package with System lib in a Virtual Environment

Details

  • Replaces sys.prefix with sys.base_prefix
    • these are different when inside a virtual environment. Using sys.base_prefix instead of sys.prefix enables installation of package in a virtual environment when the xgboost is already installed on the system

sys.base_prefix
Set during Python startup, before site.py is run, to the same value as prefix. If not running in a virtual environment, the values will stay the same; if site.py finds that a virtual environment is in use, the values of prefix and exec_prefix will be changed to point to the virtual environment, whereas base_prefix and base_exec_prefix will remain pointing to the base Python installation (the one which the virtual environment was created from).

Motivation

Setting up tests to run inside a virtual environment in an image that has xgboost pre-installed as a system lib. (e.g. nvcr.io/nvidia/tensorflow:23.06-tf2-py3). When in a virtual environment, the sys.prefix changes to point to the lib directory inside the virtualenvironment instead of the system lib path.

  • Installing xgboost with the use_system_libxgboost=True in a virtual environment
  • Using a tox virtual environment with sitepackages=true to pickup an xgboost installation present on the system already

@trivialfis trivialfis requested a review from hcho3 June 30, 2023 10:52
@oliverholworthy oliverholworthy marked this pull request as ready for review June 30, 2023 11:33
Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Thank you for the work on improved support with venv. However, I got a bit confused by the paths now. Maybe we need a new option that's more general as discussed in #6706, specifically #6706 (comment). Without a new option, it's difficult to handle the diversity of packaging methods. In the case of venv, the shared object can be in /usr/lib or myvenv/lib, or myvenv/lib/python3.x/site-packages/xgboost/lib.

Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

There are times when we need to use sys.prefix. Example: libxgboost is installed inside a Conda enviornment. This PR will break the conda-forge recipe for XGBoost.

@trivialfis
Copy link
Member

I think mamba always copies the python executable, so the base_prefix and the prefix are the same:

Python 3.10.12 | packaged by conda-forge | (main, Jun 23 2023, 22:40:32) [GCC 12.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.base_prefix
'/home/jiamingy/.anaconda/envs/xgboost_dev'
>>> sys.prefix
'/home/jiamingy/.anaconda/envs/xgboost_dev'

I expect conda to have the same behavior.

@hcho3
Copy link
Collaborator

hcho3 commented Jun 30, 2023

It would be nice to make paths user configurable

Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

I realize that this PR doesn't break Conda. For now this change is fine. In a future PR, we can add user configuration for paths.

@trivialfis
Copy link
Member

I canceled the job for the stalling test. It's caused by an issue in column-split test, which has been fixed in master branch. Will rebase the PR.

@trivialfis trivialfis merged commit 6c9c8a9 into dmlc:master Jul 4, 2023
@trivialfis
Copy link
Member

trivialfis commented Jul 11, 2023

@hcho3 @oliverholworthy After reading the mamba document: https://mamba.readthedocs.io/en/latest/advanced_usage/more_concepts.html#linking I think this PR might be problematic if there are users who prefer soft-links for working across file system partitions.

@hcho3
Copy link
Collaborator

hcho3 commented Jul 11, 2023

Thanks, I opened #9376 as a reminder.

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.

3 participants