read file in read mode

[BrunoBelucci]

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

[PGijsbers]

Thanks for catching this issue and setting up a pull request! There is an issue with casting avoid_duplicate_runs into a boolean, but otherwise it looks good! Would you be able to resolve that? Something like config.get("avoid_duplicate_runs", "").lower() == "true" is good enough for now I think.

[BrunoBelucci]

@PGijsbers it is done =)

[PGijsbers]

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?

[PGijsbers]

Changing my review status to indicate my earlier concerns are addressed and to make sure it can be merged later. Still need approval of a 2nd reviewer before merging as I now contributed to this PR.

[LennartPurucker]

LGTM!

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.

[LennartPurucker]

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

[PGijsbers]

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]

I agree, feel free to merge!

[PGijsbers]

Thanks again @BrunoBelucci 👏

[LennartPurucker]

Thank you @BrunoBelucci!