Skip to content

245 support the new version of khiops env#252

Merged
popescu-v merged 12 commits intodevfrom
245-support-the-new-version-of-khiops_env
Oct 14, 2024
Merged

245 support the new version of khiops env#252
popescu-v merged 12 commits intodevfrom
245-support-the-new-version-of-khiops_env

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

2 participants

@popescu-v @folmos-at-orange