[ENH] Improve Global Config Architecture

[SimonBlanke]

The config module uses extensive globals and I agree with this comment: https://github.com/openml/openml-python/blob/main/openml/config.py#L395

This is problematic for the following reasons:

• Any code can modify openml.config.apikey = "x". So bad or no encapsulation
• Global state persists between tests, causing flaky tests
• openml.config.apikey = 123 silently accepts wrong type. So no validation

And I am sure there are more reasons.

I think it was done to have this kind of API instead of function-call-like syntax:

openml.config.apikey = "my-key"
openml.config.server = "https://test.openml.org/api/v1/xml"

We can preserve this API and still get rid of most of these globals()/global, by defining a module level __getattr__ and __setattr__ and use a dataclass for the data encapsulation and validation:

  from dataclasses import dataclass, replace

  @dataclass
  class OpenMLConfig:
      apikey: str = ""
      server: str = "https://www.openml.org/api/v1/xml"
      # ... 

  _config = OpenMLConfig()

  def __getattr__(name: str):
      if hasattr(_config, name):
          return getattr(_config, name)
      raise AttributeError(f"module 'openml.config' has no attribute '{name}'")

  def __setattr__(name: str, value):
      global _config
      if hasattr(_config, name):
          _config = replace(_config, **{name: value})
      else:
          raise AttributeError(f"module 'openml.config' has no attribute '{name}'")

This is still not a solution I am completely content with, but it is an improvement and not too difficult to implement.

[satvshr]

Picking this one up. Given I also wanted to look more into architecture design, I will try to figure out a better approach too given you aren't happy with the fix you suggested either!

[satvshr]

@SimonBlanke I decided to implement your approach right now and work on my own architecture for config separately. I had a question to ask:
1.

• So no validation

What kind of validation should exist? Essentially, do you mean:

• syntactic validation (e.g. format / length / allowed characters), or
• semantic validation (e.g. detecting expired / revoked keys via the server)?
  If there are known rules for API key validity, could you share them? Or a set of expired keys to infer rules from?

---

IGNORE FOR NOW:
2.

• Global state persists between tests, causing flaky tests

I’m currently looking into fixing this by scoping the API key using ContextVar, so that any test-specific API key override is automatically isolated to that test.

If this approach is acceptable, should I:

• update existing / legacy tests to use this scoped behavior, or
• add new tests that explicitly verify the temporary nature of the API key?

[satvshr]

def setattr(name: str, value):

Cannot be used at a modular level, refer here where the python council rejects a proposal to add that for python 3.13

[PGijsbers]

I haven't had a lot of sleep so forgive me if I am wrong but we could internally refactor the config module into e.g., _config (or something else) and have openml.config actually expose an instantiated class? Then we can have validation quite trivially.

But we've already discussed that API redesigns are OK so we can work towards something that avoids global state (and so also helps with making tests independent).

[satvshr]

I haven't had a lot of sleep so forgive me if I am wrong but we could internally refactor the config module into e.g., _config (or something else) and have openml.config actually expose an instantiated class? Then we can have validation quite trivially.

My question was more to do with "what kind of validation do we need?" rather than "how to validate (config vars)?". I still have no idea what kind of validation we need as mentioned above:

What kind of validation should exist? Essentially, do you mean:

• syntactic validation (e.g. format / length / allowed characters), or
• semantic validation (e.g. detecting expired / revoked keys via the server)?
  If there are known rules for API key validity, could you share them? Or a set of expired keys to infer rules from?

ps: @PGijsbers given __setattr__ does not work on a modular level, I guess we also have to figure out "how to validate?" too :D

---

But we've already discussed that API redesigns are OK so we can work towards something that avoids global state (and so also helps with making tests independent).

I will be working on a redesign while the current proposed fix temporarily exist.

[SimonBlanke]

@satvshr

What kind of validation should exist?

I would start with simple input type validation (via __post_init__ or setter?) or checks like string-length for the api-key.

[SimonBlanke]

I haven't had a lot of sleep so forgive me if I am wrong but we could internally refactor the config module into e.g., _config (or something else) and have openml.config actually expose an instantiated class? Then we can have validation quite trivially.

I like this, but maybe we can do something like this (in config.py) instead of creating an instance:

sys.modules[__name__].__class__ = _ConfigModule

This way openml.config behaves like a module (like a user would expect). The input validation can then be done in the setter-methods. @fkiraly would you like this solution or should we rather go with your proposal: #1577 (comment) ?

But we've already discussed that API redesigns are OK so we can work towards something that avoids global state (and so also helps with making tests independent).

I agree in the long term. But having a solution, that improves this without a breaking change is still good for the next minor version bump. This way we get some quick improvements.

[satvshr]

I like this, but maybe we can do something like this (in config.py) instead of creating an instance:

sys.modules[name].class = _ConfigModule
This way openml.config behaves like a module (like a user would expect). The input validation can then be done in the setter-methods.

This is what @fkiraly had suggested:

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).

Which one should I move forward with? I think it should be with Franz's version given (in Simons approach) the use of sys seems more invasive, and the method unconventional.

[satvshr]

An update @fkiraly @SimonBlanke, one thing I missed with Franz's approach was that if we end up making the OpenMLConfig instance as the only variable being exported from config.py, thus we will be ignoring all the other functions and variables being used globally currently, and we will have to change their calls to reflect the changes to config.py. For example, the call to openml.config.get_server_base_url where get_server_base_url earlier was a function exported with the module, we know would have to change it to openml._config.get_server_base_url to reference the function, given openml.config is now being routed to the instance of the OpenMLConfig class.

[fkiraly]

This way openml.config behaves like a module (like a user would expect). The input validation can then be done in the setter-methods. @fkiraly would you like this solution or should we rather go with your proposal: #1577 (comment)

@SimonBlanke, I would prefer not to touch sys.modules at all - manipulating module discovery can have nasty side effects that are hard to predict.

I think the proposal of @PGijsbers of redirecting to a class is much cleaner. I have spelled it out in my comment in #1577.

[fkiraly]

An update @fkiraly @SimonBlanke, one thing I missed with Franz's approach was that

I am not quite sure how you arrive at the conclusion above. All contents of the config module are replaced with an instance of the OpenMLConfig class, which should expose any fields the config module previously had.

[satvshr]

An update @fkiraly @SimonBlanke, one thing I missed with Franz's approach was that

I am not quite sure how you arrive at the conclusion above. All contents of the config module are replaced with an instance of the OpenMLConfig class, which should expose any fields the config module previously had.

Oh ok, I thought the dataclass was only for the global variables mentioned in the architecture suggested in the issue. Will be doing what you said above now.