Skip to content

245 support the new version of khiops env #252

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.

Sign up for GitHub

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

Jump to bottom
Merged
popescu-v merged 12 commits into from
Oct 14, 2024
Merged

245 support the new version of khiops env #252

popescu-v merged 12 commits into from
Oct 14, 2024

Conversation

@popescu-v
Copy link
Collaborator

@popescu-v popescu-v commented Oct 7, 2024

No description provided.

@popescu-v popescu-v added this to the 10.2.3.0 milestone Oct 7, 2024
@popescu-v popescu-v self-assigned this Oct 7, 2024
@popescu-v popescu-v linked an issue Oct 7, 2024 that may be closed by this pull request
Support the new version of khiops_env #245
Closed
folmos-at-orange
folmos-at-orange requested changes Oct 8, 2024
View reviewed changes
@popescu-v popescu-v force-pushed the 245-support-the-new-version-of-khiops_env branch 22 times, most recently from 7eafdbd to 377f93a Compare October 10, 2024 08:17
folmos-at-orange
folmos-at-orange requested changes Oct 10, 2024
View reviewed changes
@popescu-v popescu-v force-pushed the 245-support-the-new-version-of-khiops_env branch 2 times, most recently from bca99b4 to bdda12b Compare October 10, 2024 11:16
folmos-at-orange
folmos-at-orange requested changes Oct 10, 2024
View reviewed changes
Copy link
Member

@folmos-at-orange folmos-at-orange left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All minor stuff.

However, it doesn't work in macOS. A change in khiops_env is necessary.

Update:

I'd also make private get_linux_distribution_name , check_samples_dir and get_dir_status so they dont pollute the documentation.

@popescu-v
Copy link
Collaborator Author

popescu-v commented Oct 10, 2024

All minor stuff.

However, it doesn't work in macOS. A change in khiops_env is necessary.

Update:

I'd also make private get_linux_distribution_name , check_samples_dir and get_dir_status so they dont pollute the documentation.

  • get_linux_distribution_name was moved to test_khiops_integrations.py and made private therein; it is no longer necessary in the runner code (as those variables are set by khiops_env now).
  • the two other methods were made private.

@popescu-v popescu-v force-pushed the 245-support-the-new-version-of-khiops_env branch 2 times, most recently from 8abc494 to 1933dc6 Compare October 11, 2024 17:45
folmos-at-orange
folmos-at-orange approved these changes Oct 14, 2024
View reviewed changes
@popescu-v popescu-v force-pushed the 245-support-the-new-version-of-khiops_env branch 2 times, most recently from e28fb9d to ec07ebe Compare October 14, 2024 10:22
folmos-at-orange
folmos-at-orange approved these changes Oct 14, 2024
View reviewed changes
@popescu-v
Update KhiopsLocalRunner to use the Khiops 10.2.3 khiops_env script
fa499d7 
Thusly, various `KhiopsLocalRunner` attributes' values are
sourced from the `khiops_env` script on all platforms:

- `mpi_command_args`: set according to `KHIOPS_MPI_COMMAND`
- `max_cores`: set according to `KHIOPS_PROC_NUMBER`
   Note: as `KHIOPS_MPI_COMMAND` depends on `KHIOPS_PROC_NUMBER`,
   `max_cores` cannot be updated directly on an existing
   `KhiopsLocalRunner` instance; to update this parameter, one needs to:
   1. set the `KHIOPS_PROC_NUMBER` environment variable
   2. create a new `KhiopsLocalRunner` instance
- `max_memory_mb`: set according to `KHIOPS_MEMORY_LIMIT`
- `khiops_temp_dir`: set according to `KHIOPS_TMP_DIR`
- `khiops_path`: set according to `KHIOPS_PATH`
- `khiops_coclustering_path`: set according to `KHIOPS_COCLUSTERING_PATH`

This change also entails that:
- the following environment variables are no longer used:
 - `KHIOPS_MPI_COMMAND_ARGS`
 - `KHIOPS_MPIEXEC_PATH`
 - `KHIOPS_MPI_LIB`
 - `KHIOPS_HOME` on Windows operating systems
- the MPI command can be further customized, on distributed environments
  only, according to the new `khiops_env` script, via the
  `KHIOPS_MPI_HOST_FILE` environment variable, that is only used by
  `khiops_env` for customizing the MPI command
- the MPI command is exposed as the `KHIOPS_MPI_COMMAND` environment
  variable that is set by the `khiops_env` script and cannot be changed
  by the user directly
@popescu-v
Update estimator checks for sklearn >= 1.5
8aecaa6
@popescu-v popescu-v force-pushed the 245-support-the-new-version-of-khiops_env branch from ec07ebe to baeb172 Compare October 14, 2024 11:07
@popescu-v
Just look for mpiexec in kh-status
45c5078 
`bin` is not necessarily present; `mpiexec` can be anywhere.
@popescu-v
Initialize Khiops environment upon creating KhiopsLocalRunner instance
1bfdef4 
Namely:

- call `_initialize_khiops_environment` only once, in
  `KhiopsLocalRunner`'s `__init__` method

- initially set the global `_khiops_runner` to `None`

- in the `get_runner` function:
  - if the global `khiops_runner` is `None`, then:
    - create `KhiopsLocalRunner` instance, which initializes the
      environment
    - set the global `_khiops_runner` to the `KhiopsLocalRunner` instance
  - return `_khiops_runner`, as before

Thusly, `KhiopsLocalRunner`'s environment is fully initialized upon
creation.

Only `khiops_version` is initialized lazily because this needs
a call to Khiops binaries, which incurs a time / performance cost and
hence is done only when needed.
@popescu-v
Keep vanilla PATH as on the supported Linux OSes
a5be89c 
This path change was meant to test issue
#209, which is moot in
our case, because:

- all currently supported Linux distributions (Ubuntu 20.04, 22.04, 24.04;
  Rocky 8, 9; Debian 10, 11, 12) have `/usr/bin` before `/bin` in `PATH`
- the Arch Linux distribution that is mentioned in the above-cited issue
  is not currently supported.
@popescu-v
Handle the Windows native + pip install case via KHIOPS_HOME
6a3f72f
@popescu-v
Add the binary status to the library status
018558c
@popescu-v
Add Windows integration check CI job
7d01a8d
@popescu-v
Set KHIOPS_PROC_NUMBER > 2 so that mpiexec gets set to non-empty string
f2887c3
@popescu-v
Update Conda dependency on khiops-core to 10.2.3b.4
6942fd9 
Thus, we ensure a `khiops-core` version that uses the updated
`khiops_env` is installed.
@popescu-v
Add conda-forge to Windows test workflow (to comply with upstream khi…
81acfae 
…ops-core)

TODO: Revert this just before merging PR #223
@popescu-v
Update default Khiops Core version in the CI for tests to 10.2.3[-]b.4
91ccb46 
TODO: Revert after:

- building `latest` on `dev` with the proper `10.2.3` Khiops version
  (once the Khiops 10.2.3 release is done)
- merging this PR
@popescu-v popescu-v force-pushed the 245-support-the-new-version-of-khiops_env branch from baeb172 to 91ccb46 Compare October 14, 2024 12:48
@popescu-v popescu-v merged commit c2cb69d into dev Oct 14, 2024
@popescu-v popescu-v deleted the 245-support-the-new-version-of-khiops_env branch October 14, 2024 13:52
@popescu-v popescu-v mentioned this pull request Oct 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@folmos-at-orange folmos-at-orange folmos-at-orange approved these changes

Assignees

@popescu-v popescu-v

Labels

None yet

Projects

None yet

Milestone

10.2.3.0

Development

Successfully merging this pull request may close these issues.

Support the new version of khiops_env

3 participants

@popescu-v @folmos-at-orange