[ENH] Improve Global Config Architecture

[satvshr]

Metadata

• Reference Issue: fixes [ENH] Improve Global Config Architecture #1564
• New Tests Added: No
• Documentation Updated: No
• Change Log Entry: Replaces global variables with an instance of a dataclass OpenMLConfig.

Details

• What does this PR implement/fix? Explain your changes.

  • Mainly Introduces an OpenMLConfig dataclass and a single module-level instance _config in openml.config.
  • Reads/parses configuration defaults from OpenMLConfig.
  • Adds __getattr__ so attribute reads on openml.config forward to the dataclass (preserving read compatibility).
  • Converts internal code to use openml.config._config. (many modules, tests and examples updated accordingly).

• Why is this change necessary? What is the problem it solves?

  • consolidates defaults and runtime state into one place,
  • simplifies consistent parsing and writing of the config file.

[jgyasu]

Can you please add more details in your PR description?

[satvshr]

It's not ready for review yet! Hence a draft.

[satvshr]

@SimonBlanke if we make the dataclass OpenMLConfig frozen, we will have to change everything from the current openml.config._config.cachedir = _root_cache_directory to _config = replace(_config, cachedir=_root_cache_directory), it is your call. In my opinion, it is a good idea to freeze it and use _config = replace(_config, cachedir=_root_cache_directory) everywhere (hence making it immutable) while I try to find a better fix to the problem.

For anyone else trying to help out, kindly refer to the issue attached to the PR (#1564) to get into the loop on the discussion.

ps: Even though this is a temporary fix it seems like a good step in the right direction for having a proper config. I think I would like to add and remove a few things to build on top of this once ive a better idea though (and once this PR is closed).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 67.08861% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.62%. Comparing base (8672ffb) to head (f8fbe1e).

Files with missing lines	Patch %	Lines
openml/config.py	66.12%	21 Missing ⚠️
openml/_api_calls.py	71.42%	2 Missing ⚠️
openml/cli.py	0.00%	2 Missing ⚠️
openml/runs/functions.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1577      +/-   ##
==========================================
- Coverage   52.71%   52.62%   -0.10%     
==========================================
  Files          36       36              
  Lines        4325     4321       -4     
==========================================
- Hits         2280     2274       -6     
- Misses       2045     2047       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fkiraly]

You changed the public interface to the config and apparently users have to write to a private module. That is not good design. Please leave the public APIs inteacts, otherwise this would be a breaking change.

Also, in the summary for this PR, please explain what you change and why.

[satvshr]

You changed the public interface to the config and apparently users have to write to a private module. That is not good design. Please leave the public APIs inteacts, otherwise this would be a breaking change.

I know that the public interface was changed, and that it shouldnt be changed, but the interface HAS to be changed if I follow Simon's proposed design in the issue description in #1564 given modular __setattr__ is not allowed by python, which was part of the design proposed by @SimonBlanke in the issue, hence I went forward with the "minimum" changes required for the proposed architecture, for him to take a look at and make further decisions.

[fkiraly]

I know that the public interface was changed, and that it shouldnt be changed,

It is a key requirement that it does not change. Or it will be a breaking change.

but the interface HAS to be changed if I follow Simon's proposed design in the issue description in #1564

this is wrong. I would appreciate if you would not assert this with such certainty.

It is true that modular __setattr__ does not work, but that does not mean that the design is impossible.

Also, you should be able to catch that a change in design that touches 13 files is perhaps not the right way to go.

I went forward with the "minimum" changes required for the proposed architecture, for him to take a look at and make further decisions.

No, the minimal changes imo would be the ones that preserve the current API and stay close to @SimonBlanke´s proposal, instead of modifying 13 files. Changing 13 files is not "minimal" in the sense of scope and cohesion.

You should have pointed out the issue with __setattr__ in the issue, btw.

Anyway, the way to go imo (in order to be faithful to @SimonBlanke´s design) would be:

• turn config into a private module
• in root __init__, modify __getattr__, so that config points to a global instance of the config class
• in that class, you can override __setattr__ as well as __getattr__ (or directly have attributes written).

[fkiraly]

In terms of communication:

• you identified a problem with the design, great!
• you tried to propose a solution to the problem, great!
• however, it would have been better if you had immediately highlighted and communicated where you are at, in issue [ENH] Improve Global Config Architecture #1564:
  • that __setattr__ does not work as designed, and that you will attempt to fix the design - you communicated that, great!
  • but it would be good if you spell out the design that you had in mind, on issue [ENH] Improve Global Config Architecture #1564, once worked out. Plus pros/cons on alternatives you considered.
  • then, if no alternatives you considered, fit the key requirement (do not change public API), highlight that explicitly, since that is reaching a blocker!

[satvshr]

Also, you should be able to catch that a change in design that touches 13 files is perhaps not the right way to go.

I definitely knew it was wrong, I was going to bring it up in tomorrow's meeting, and had no intention of getting this PR merged in the current state, hence should have probably kept it in draft. Though I think I should have said that more explicity in the PR itself.

No, the minimal changes imo would be the ones that preserve the current API and stay close to @SimonBlanke´s proposal, instead of modifying 13 files. Changing 13 files is not "minimal" in the sense of scope and cohesion.

Now that there is a way around this, which involves not changing 13 files I do agree I was wrong, but just to give an idea of what was going through my head, by "minimal" I meant at that point of time to get all globals into 1 dataclass, I (felt I) had to change the 13 files to continue with Simon's design.

• it would have been better if you had immediately highlighted and communicated where you are at

I was doing that throughout no? I didn't drop an update about a better design, because I am still working on it, so there was no update about that.

• highlight that explicitly

I should have definitely done that.