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

DOC: Instructions for installing with dependencies from conda #3016

Merged

Conversation

david-cortes-intel
Copy link
Contributor

@david-cortes-intel david-cortes-intel commented Dec 10, 2024

Description

This PR adds instructions for building oneDAL from source using packages from conda to satisfy dependencies instead of using system installs. Packages from conda don't come with activation scripts, so this adds the necessary instructions for getting them to work with the oneDAL make file.

Disclaimer: I haven't fully tested the windows instructions.


PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

  • I have reviewed my changes thoroughly before submitting this pull request.
  • I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have added a respective label(s) to PR if I have a permission for that.
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.

Performance

Not applicable.

@david-cortes-intel david-cortes-intel added the docs Issue/PR related to oneDAL docs label Dec 10, 2024
INSTALL.md Outdated
@@ -35,12 +35,17 @@ Note: the Intel(R) oneAPI components listed here can be installed together throu

https://www.intel.com/content/www/us/en/developer/tools/oneapi/base-toolkit.html

These dependencies can also be installed through the `conda` software, but doing so will require a few additional setup steps - see rest of the instructions for details.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is possible to add the link to a respective chapter and remove "see rest ...".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a link.

INSTALL.md Outdated
## Docker Development Environment

[Docker file](https://github.com/uxlfoundation/oneDAL/tree/main/dev/docker) with the oneDAL development environment
is available as an alternative to the manual setup.

## Installation Steps

_(If using `conda`, see subsequent sections for instructions)_
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is needed if the reference to the chapter is added at the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

INSTALL.md Outdated
@@ -195,3 +200,69 @@ For example, in a Linux platform, assuming one wishes to execute the `adaboost_d
```shell
./_cmake_results/intel_intel64_so/adaboost_dense_batch
```

## Installation with a conda environment
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename to something like: Conda Development Environment Setup, and remove the last part about running make that duplicates the info in the previous chapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed.

INSTALL.md Outdated
Comment on lines 217 to 228
```shell
conda install -y \
-c https://software.repos.intel.com/python/conda/ \ `# Intel's repository`
-c conda-forge \ `# conda-forge, for tools like 'make'`
make python \ `# used by the build system`
dpcpp-cpp-rt dpcpp_linux-64 intel-sycl-rt \ `# Intel compiler packages`
tbb tbb-devel \ `# required TBB packages`
mkl mkl-devel mkl-static mkl-dpcpp mkl-devel-dpcpp \ `# required MKL packages`
cmake `# required to build the examples only`
```

_(for windows, replace `dpcpp_linux-64` with `dpcpp_win-64`, and remove the comments within backticks)_
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have separate instructions for Linux and Windows - save the time for users on copy-pasting.

Also, i think the minimal supported versions for the packages (at least some) need to be added. Like python=3.9, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to separate code snippets.

INSTALL.md Outdated

_(for windows, replace `dpcpp_linux-64` with `dpcpp_win-64`, and remove the comments within backticks)_

Then, one needs to modify environment variables as appropriate to point to the necessary libraries - **assuming a linux system:**
Copy link
Contributor

Choose a reason for hiding this comment

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

Other chapters use '(Linux*)' and '(Windows*)' to distinguish. Let's align and use the wording like this:

Suggested change
Then, one needs to modify environment variables as appropriate to point to the necessary libraries - **assuming a linux system:**
Modify the environment variables to point to the necessary libraries (Linux *):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Splitted into linux/windows sections.

INSTALL.md Outdated
export CMAKE_PREFIX_PATH="${CONDA_PREFIX}/lib/cmake:${CMAKE_PREFIX_PATH}"
```

Equivalent for windows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Equivalent for windows:
Modify the environment variables to point to the necessary libraries (Windows*):

Copy link
Contributor

@Vika-F Vika-F left a comment

Choose a reason for hiding this comment

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

The final request and lgtm.

INSTALL.md Outdated

Then, install the necessary dependencies from the appropriate channels with `conda`:

* Linux:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Linux:
**Linux\***:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't appear to render correctly:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified the 4 occurrences to use the formatting you're suggesting but inside bullet points.

INSTALL.md Outdated
cmake `# required to build the examples only`
```

* Windows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Windows:
**Windows\***:

INSTALL.md Outdated

Then modify the relevant environment variables to point to the conda-installed libraries:

* Linux:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Linux:
**Linux\***:

INSTALL.md Outdated
export CMAKE_PREFIX_PATH="${CONDA_PREFIX}/lib/cmake:${CMAKE_PREFIX_PATH}"
```

* windows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* windows:
**Windows\***:

@david-cortes-intel david-cortes-intel merged commit f077d53 into uxlfoundation:main Dec 18, 2024
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Issue/PR related to oneDAL docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants