Skip to content

Prepare v11 support #367

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

Merged
merged 22 commits into from
Mar 19, 2025
Merged

Prepare v11 support #367

merged 22 commits into from
Mar 19, 2025

Conversation

popescu-v
Copy link
Collaborator

@popescu-v popescu-v commented Mar 11, 2025

Closes #325, #329, #236, #241, #81, #77.
Supersedes PR #366 .


TODO Before Asking for a Review

  • Rebase your branch to the latest version of dev (or main for release PRs)
  • Make sure all CI workflows are green
  • When adding a public feature/fix: Update the Unreleased section of CHANGELOG.md (no date)
  • Self-Review: Review "Files Changed" tab and fix any problems you find
  • API Docs (only if there are changes in docstrings, rst files or samples):
    • Check the docs build without warning: see the log of the API Docs workflow
    • Check that your changes render well in HTML: download the API Docs artifact and open index.html
    • If there are any problems it is faster to iterate by building locally the API Docs

@popescu-v popescu-v mentioned this pull request Mar 11, 2025
3 tasks
@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from 0705f18 to 7917714 Compare March 12, 2025 19:48
@popescu-v popescu-v self-assigned this Mar 12, 2025
@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from 7917714 to c3df671 Compare March 12, 2025 20:16
Copy link
Member

@folmos-at-orange folmos-at-orange left a comment

Choose a reason for hiding this comment

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

Lots of small stuff. A few not so small.

@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from c3df671 to 524f8be Compare March 13, 2025 20:10
Copy link
Member

@folmos-at-orange folmos-at-orange left a comment

Choose a reason for hiding this comment

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

Second round.

@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from 524f8be to fb040a1 Compare March 14, 2025 11:53
@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from fb040a1 to 59dd770 Compare March 14, 2025 13:43
Copy link
Member

@folmos-at-orange folmos-at-orange left a comment

Choose a reason for hiding this comment

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

Found with the typos tool:

error: `intepretation` should be `interpretation`
  --> khiops/samples/samples.py:788:15
    |
788 |     """Builds intepretation model for existing predictor
    |               ^^^^^^^^^^^^^
    |
error: `intepretation` should be `interpretation`
  --> khiops/samples/samples.py:815:17
    |
815 |     print(f"The intepretation model is '{interpretor_file_path}'")
    |                 ^^^^^^^^^^^^^
    |
error: `intepretation` should be `interpretation`
  --> khiops/samples/samples.ipynb:835:13
    |
835 |     "Builds intepretation model for existing predictor\n\n    It calls `~.api.train_predictor` and `~.api.interpret_predictor` only with\n    their mandatory parameters.\n    \n"
    |             ^^^^^^^^^^^^^
    |
error: `intepretation` should be `interpretation`
  --> khiops/samples/samples.ipynb:866:19
    |
866 |     "print(f\"The intepretation model is '{interpretor_file_path}'\")"
    |                   ^^^^^^^^^^^^^
    |
error: `Intepretation` should be `Interpretation`
  --> khiops/core/internals/tasks/interpret_predictor.py:36:12
   |
36 |         // Intepretation settings
   |            ^^^^^^^^^^^^^
   |
error: `intepretation` should be `interpretation`
  --> khiops/core/internals/tasks/interpret_predictor.py:50:18
   |
50 |         // Build intepretation dictionary
   |                  ^^^^^^^^^^^^^
   |
error: `intepret` should be `interpret`
  --> khiops/core/api.py:318:35
    |
318 |                 "'khiops.core.api.intepret_predictor' function is not supported "
    |                                   ^^^^^^^^
    |
error: `intepretation` should be `interpretation`
  --> khiops/core/api.py:825:19
    |
825 |     r"""Builds an intepretation dictionary from a predictor
    |                   ^^^^^^^^^^^^^
    |
error: `intepretor` should be `interpreter`
  --> khiops/core/api.py:833:5
    |
833 |     intepretor_file_path : str
    |     ^^^^^^^^^^
    |
error: `intepretor` should be `interpreter`
  --> khiops/core/api.py:834:21
    |
834 |         Path to the intepretor dictionary file.
    |                     ^^^^^^^^^^
    |
error: `intepretation` should be `interpretation`
  --> khiops/core/api.py:836:70
    |
836 |         Maximum number of variable importances to be selected in the intepretation
    |                                                                      ^^^^^^^^^^^^^
    |
error: `intepretation` should be `interpretation`
  --> khiops/core/api.py:845:9
    |
845 |         intepretation model. Min length: 0. Max length: the total number of variables
    |         ^^^^^^^^^^^^^
    |
error: `intialization` should be `initialization`
  --> tests/test_khiops_integrations.py:202:51
    |
202 |         # Get KHIOPS_API_MODE as set after runner intialization
    |                                                   ^^^^^^^^^^^^^
    |
error: `overriden` should be `overridden`
  --> tests/test_remote_access.py:104:22
    |
104 |             """To be overriden by descendants"""
    |                      ^^^^^^^^^
    |

After this it should be ok.

@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from 59dd770 to 27f6d7e Compare March 14, 2025 19:03
Copy link
Member

@folmos-at-orange folmos-at-orange left a comment

Choose a reason for hiding this comment

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

2 3 minor things.

And make sure pre-commit passes.

@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch 3 times, most recently from 32620bf to 4a936d8 Compare March 18, 2025 12:58
@popescu-v
Copy link
Collaborator Author

popescu-v commented Mar 18, 2025

@folmos-at-orange : Please check commit 8e6c19f.

Copy link
Member

@folmos-at-orange folmos-at-orange left a comment

Choose a reason for hiding this comment

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

LGTM but unless you fix pylint you won't be able to merge (pre-commit it is the only required check).

- drop deprecated arguments
- add new arguments
- deprecate results_dir
- drop all previous v11 deprecations
If a log file path is supplied by the user, then keep the log for
subsequent user inspection.
…aries

Deprecate previous data-path '`'-based convention.
Indeed, backward compatibility with pre-v11 Khiops versions has been
dropped.
Thusly, stop disambiguating Khiops paths, as the API mode ensures that
paths can only be relative to the current working directory.
Only keep list-like, convertible to NumPy arrays, input observables.
- propagate Core API updates to parameter passing tests
- propagate secondary-table path update to sklearn tests
- update Core API calls to the v11 Core API
- drop all sklearn-specific deprecated Khiops estimator fields
- drop deprecated `key` dataset attribute
- take into account dataset deprecation drops
- drop deprecated sklearn samples
Thus, the baseline model (starting with (B_)) is taken account of, but
is not used (so that regressions are not entailed).
See the comment
KhiopsML/khiops#626 (comment) of
Khiops core issue KhiopsML/khiops#626 for the
full rationale.
Also update the testing Docker images accordingly.
@popescu-v popescu-v force-pushed the 325-prepare-v11-support branch from 774517d to c624616 Compare March 19, 2025 10:02
@popescu-v popescu-v merged commit 82ee065 into dev Mar 19, 2025
151 of 152 checks passed
@popescu-v popescu-v deleted the 325-prepare-v11-support branch March 19, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants