-
Notifications
You must be signed in to change notification settings - Fork 717
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 uv tool for isolate cicd env #2597
base: development
Are you sure you want to change the base?
Conversation
b4c85f0
to
3b2414b
Compare
1.AI-and-Analytics/End-to-end-Workloads/JobRecommendationSystem 2.AI-and-Analytics/Features-and Functionality/INC_QuantizationAwareTraining_TextClassification 3.AI-and-Analytics/Features-and-Functionality/Intel_Extension_For_SKLearn_Performance_SVC_Adult 4.AI-and-Analytics/Features-and-Functionality/IntelPython_daal4py_DistributedKMeans 5.AI-and-Analytics/Features-and-Functionality/IntelPython_daal4py_DistributedLinearRegression 6.AI-and-Analytics/Features-and-Functionality/IntelPython_GPU_dpnp_Genetic_Algorithm 7.AI-and-Analytics/Features-and-Functionality/IntelPython_Numpy_Numba_dpnp_kNN 8.AI-and-Analytics/Features-and-Functionality/IntelPython_XGBoost_Performance 9.AI-and-Analytics/Features-and-Functionality/IntelPyTorch_GPU_InferenceOptimization_with_AMP 10.AI-and-Analytics/Features-and-Functionality/IntelPyTorch_TrainingOptimizations_AMX_BF16 11.AI-and-Analytics/Features-and-Functionality/IntelTensorFlow_AMX_BF16_Inference 12.AI-and-Analytics/Features-and-Functionality/IntelTensorFlow_AMX_BF16_Training 13.AI-and-Analytics/Features-and-Functionality/IntelTensorFlow_Enabling_Auto_Mixed_Precision_for_TransferLearning 14.AI-and-Analytics/Features-and-Functionality/IntelTensorFlow_for_LLMs 15.AI-and-Analytics/Features-and-Functionality/IntelTensorFlow_TextGeneration_with_LSTM 16.AI-and-Analytics/Features-and-Functionality/IntelTransformers_Quantization 17.AI-and-Analytics/Getting-Started-Samples/INC-Quantization-Sample-for-PyTorch 18.AI-and-Analytics/Getting-Started-Samples/INC-Sample-for-Tensorflow 19.AI-and-Analytics/Getting-Started-Samples/Intel_Extension_For_SKLearn_GettingStarted 20.AI-and-Analytics/Getting-Started-Samples/Intel_Extension_For_TensorFlow_GettingStarted 21.AI-and-Analytics/Getting-Started-Samples/IntelPython_daal4py_GettingStarted 22.AI-and-Analytics/Getting-Started-Samples/IntelPython_XGBoost_GettingStarted 23.AI-and-Analytics/Getting-Started-Samples/Modin_GettingStarted 24.AI-and-Analytics/Getting-Started-Samples/Modin_Vs_Pandas Signed-off-by: Xu, He <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments mostly around:
- Activating conda environment
- ipykernel as dev dependency
- Why is numpy installed separately?
Furthermore, I don't see pyproject.toml
and uv.lock
files. The reproducibility aspect of UV is totally missed, this PR only uses UV as a pip replacement. Ideally, we want to use UV as a complete package manager.
"pip install -r requirements.txt", | ||
"python -m ipykernel install --user --name=user_tensorflow-gpu", | ||
"python JobRecommendationSystem.py" | ||
"conda activate tensorflow-gpu", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to activate conda environment? As far as I know, UV will create an isolated environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reply at below
"uv add seaborn", | ||
"uv add numpy==1.26.4", | ||
"uv run python -m spacy download en_core_web_sm", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit but can have them in a single uv add
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored
"uv add --dev ipykernel", | ||
"uv run ipython kernel install --user --name=pytorch-gpu", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to install ipykernel if we not using it. I know its there in current JSON file, but its good opportunity to clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
"uv python pin $(which python)", | ||
"uv venv --system-site-packages", | ||
"uv add -r requirements.txt", | ||
"uv add --dev ipykernel notebook", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please install ipykernel in standard dependency? No need for --dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed --dev
"export execution_mode=eager", | ||
"uv run python Intel_TensorFlow_AMX_BF16_Training.py -m eager", | ||
"uv run ipython kernel install --user --name tensorflow", | ||
"uv run jupyter nbconvert --ExecutePreprocessor.enabled=True --ExecutePreprocessor.kernel_name=tensorflow --to notebook IntelTensorFlow_AMX_BF16_Training.ipynb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need kernel_name argument any more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, if the .ipynb file not specify the kernel name and command line also not include it, we might meet issue like jupyter_client.kernelspec.NoSuchKernel: No such kernel named tf_amx_bf16_2
, maybe keep the arg for now.
"conda activate tensorflow-gpu", | ||
"pip install uv notebook", | ||
"uv init", | ||
"uv python pin $(which python)", | ||
"uv venv --system-site-packages", | ||
"uv add -r requirements.txt", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, notebook is install separately. Both notebook and ipykernel are not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, removed
"pip install notebook", | ||
"uv add --dev ipykernel", | ||
"uv run ipython kernel install --user --name tensorflow" | ||
|
||
], | ||
"id": "neural-compressor tensorflow", | ||
"steps": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you missed uv run
in the next line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, thanks !
"conda activate base", | ||
"pip install uv notebook", | ||
"uv init", | ||
"uv python pin $(which python)", | ||
"uv venv --system-site-packages", | ||
"uv add -r requirements.txt", | ||
"uv add numpy==1.26.4", | ||
"uv run ipython kernel install --user --name=tensorflow", | ||
"uv run ipython kernel install --user --name=tensorflow-gpu", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original sample.json file tried to test the sample in both CPU and GPU. This doesn't seems to be doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, already remove cpu one and keep the gpu one
"id": "Intel_Modin_GS_py", | ||
"steps": [ | ||
"source /intel/oneapi/intelpython/bin/activate", | ||
"pip install -r requirements.txt", | ||
"pip install jupyter ipykernel", | ||
"jupyter nbconvert --ExecutePreprocessor.enabled=True --to notebook Modin_GettingStarted.ipynb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed uv run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, added
Getting-Started-Samples/INC-Quantization-Sample-for-PyTorch Getting-Started-Samples/INC-Sample-for-Tensorflow Getting-Started-Samples/IntelPython_daal4py_GettingStarted Getting-Started-Samples/Intel_Extension_For_SKLearn_GettingStarted Getting-Started-Samples/Intel_Extension_For_TensorFlow_GettingStarted Getting-Started-Samples/Modin_GettingStarted Getting-Started-Samples/Modin_Vs_Pandas
Features-and-Functionality/IntelPyTorch_GPU_InferenceOptimization_with_AMP Features-and-Functionality/IntelPyTorch_TrainingOptimizations_AMX_BF16 Features-and-Functionality/IntelTensorFlow_AMX_BF16_Inference Features-and-Functionality/IntelTensorFlow_AMX_BF16_Training Features-and-Functionality/IntelTensorFlow_Enabling_Auto_Mixed_Precision_for_TransferLearning Features-and-Functionality/IntelTensorFlow_TextGeneration_with_LSTM Features-and-Functionality/IntelTensorFlow_for_LLMs Features-and-Functionality/IntelTransformers_Quantization
The reasons why we use AI Tool's Conda environments alongside uv:
|
ipykernel issue is resolved by removing --dev |
This PR isn’t just about using |
Thanks for the review @Ankur-singh , we refactor the code and added |
End-to-end-Workloads/JobRecommendationSystem Features-and Functionality/INC_QuantizationAwareTraining_TextClassification Features-and-Functionality/Intel_Extension_For_SKLearn_Performance_SVC_Adult Features-and-Functionality/IntelPython_daal4py_DistributedKMeans Features-and-Functionality/IntelPython_daal4py_DistributedLinearRegression Features-and-Functionality/IntelPython_GPU_dpnp_Genetic_Algorithm Features-and-Functionality/IntelPython_Numpy_Numba_dpnp_kNN Features-and-Functionality/IntelPython_XGBoost_Performance Signed-off-by: troy818 <[email protected]>
This makes sense. The PR looks much better now. Thank you so much for all the work. There is one major question that needs to be answered. Next oneAPI release for AI tools will not have offline installer. All the packages will either be distributed via
We will have to give some thought. I will be happy to hear your ideas. cc @jimmytwei |
Oh, I see! I wasn’t aware that the Conda environment would be removed in the next AI Tool release. If that’s the case, the changes in this PR will be suitable for the current release but could also serve as a reference for the next version. For each sample, use |
I'm not sure if all the required packages for each sample can already be installed via |
Yes, I think we should be doing it incrementally.
I'm not sure about it either, we will have to wait for the next release. @jimmytwei do you have any insights about this? |
Existing Sample Changes
Description
The UV tool is designed to streamline the management of Python environments for multiple test cases. One of its standout features is its ability to operate without altering the existing Python environment, ensuring that the each unit test sample (sample.json) won't affect primary setup (AI Tool did with conda environment). This makes it an ideal solution without the hassle of environment conflicts or dependencies issues, and makes each sample isolation.
add uv tool management for samples:
Type of change
Please delete options that are not relevant. Add a 'X' to the one that is applicable.
ONSAM 1917