search

openml / openml-python

Python module to interface with OpenML
https://openml.github.io/openml-python/main/
Other
276 stars 142 forks source link

read file in read mode #1338

Closed BrunoBelucci closed 2 months ago

BrunoBelucci commented 2 months ago

Reference Issue

Fixes #1337

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

It changes the mode that config_file is being opened, so it will not be overwritten. It also casts the variables of the config_file to the expected types, otherwise, everything is read as str.

How should this PR be tested?

1 - Create a new config file .../.config/openml/config 2 - do import openml 3 - check that the config file is preserved

BrunoBelucci commented 2 months ago

@PGijsbers it is done =)

PGijsbers commented 2 months ago

Thanks! Found I had missed something, so I changed it a little after all. Also added a test.

@LennartPurucker I know this isn't very pretty. But I don't think it will be without overwriting more of the config module which I would be in favor of but shouldn't be in this PR. Since I touched this PR now, could you sign off on it if you agree that we should apply the patch?

LennartPurucker commented 2 months ago

The tests seem to be failing due to a (test) server error.

PGijsbers commented 2 months ago

The change from avoid_duplicate_runs = config.get("avoid_duplicate_runs", False) to avoid_duplicate_runs = config["avoid_duplicate_runs"] should not break since the config object must have avoid_duplicate_runs set anyhow.

Yes this was deliberate. As for the tests failing... I know :( I don't know how easy it is to resolve that. Despite that, I would still be in favor of merging it. These changes are easy to understand and solve a bug which breaks a core feature.

LennartPurucker commented 2 months ago

I agree, feel free to merge!

PGijsbers commented 2 months ago

Thanks again @BrunoBelucci 👏

LennartPurucker commented 2 months ago

Thank you @BrunoBelucci!