Skip to content

Support the new version of khiops_env #245

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

Closed
folmos-at-orange opened this issue Sep 12, 2024 · 3 comments · Fixed by #252
Closed

Support the new version of khiops_env #245

folmos-at-orange opened this issue Sep 12, 2024 · 3 comments · Fixed by #252
Assignees
Labels
Status/Done The issue has been addressed and merged to the dev branch
Milestone

Comments

@folmos-at-orange
Copy link
Member

folmos-at-orange commented Sep 12, 2024

Description

The khiops_env PR will land soon KhiopsML/khiops#343. We should support the new features of khiops_env and its availability in the conda installation.

Questions/Ideas

  • Make the conda detection use khiops_env
    • Conditional code?
  • Support all the available variables in the new khiops_env
  • "Deprecate" the old code
@folmos-at-orange folmos-at-orange added Status/ReadyForDev The issue is ready to be developed or to be investigated deeply Type/DevChore Priority/0-High To do now labels Sep 12, 2024
@folmos-at-orange folmos-at-orange self-assigned this Sep 12, 2024
@folmos-at-orange folmos-at-orange changed the title Support the new version of khiops_env Support the new version of khiops_env Sep 12, 2024
@popescu-v
Copy link
Collaborator

AFAIU, it would suffice to just initialize the relevant attributes of the KhiopsLocalRunner with the values of the appropriate environment variables, as sourced from khiops_env, irrespective of the environment. The new khiops_env now takes care of appropriately setting all relevant environment variables according to the execution environment (Conda, Conda-based, native), in a robust way, without (much) environment-conditional code.

@popescu-v
Copy link
Collaborator

Also, as the old code was not part of any official API, all deprecation mechanics would not be necessary IMHO; it would just suffice to remove that (environment-conditional) code.

@folmos-at-orange folmos-at-orange removed their assignment Sep 23, 2024
@lucaurelien lucaurelien added this to the 10.2.3.0 milestone Sep 26, 2024
@popescu-v popescu-v self-assigned this Sep 30, 2024
@popescu-v
Copy link
Collaborator

AFAIU, it would suffice to just initialize the relevant attributes of the KhiopsLocalRunner with the values of the appropriate environment variables, as sourced from khiops_env, irrespective of the environment. The new khiops_env now takes care of appropriately setting all relevant environment variables according to the execution environment (Conda, Conda-based, native), in a robust way, without (much) environment-conditional code.

There is one caveat though: we should allow users to override even the variables that are already set in the khiops_env script:

  • source khiops_env variables into a specific data structure
  • for each of these variables, if the variable is already in os.environ, use the value from the environment. Otherwise, use the khiops_env-defined value.

Thusly, current behavior whereby user-defined environment variables take precedence over default values (henceforth defined in khiops_env) is preserved.

@popescu-v popescu-v linked a pull request Oct 7, 2024 that will close this issue
@folmos-at-orange folmos-at-orange added Status/InDevelopment The issue is in development by one or more team members Status/Done The issue has been addressed and merged to the dev branch and removed Status/ReadyForDev The issue is ready to be developed or to be investigated deeply Priority/0-High To do now Status/InDevelopment The issue is in development by one or more team members labels Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status/Done The issue has been addressed and merged to the dev branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants