Skip to content
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

read file in read mode #1338

Merged
merged 4 commits into from
May 15, 2024
Merged

read file in read mode #1338

merged 4 commits into from
May 15, 2024

Conversation

BrunoBelucci
Copy link
Contributor

@BrunoBelucci BrunoBelucci commented May 12, 2024

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

Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

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.

openml/config.py Outdated Show resolved Hide resolved
@BrunoBelucci
Copy link
Contributor Author

@PGijsbers it is done =)

@PGijsbers
Copy link
Collaborator

PGijsbers commented May 14, 2024

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?

Copy link
Collaborator

@PGijsbers PGijsbers left a comment

Choose a reason for hiding this comment

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

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.

@PGijsbers PGijsbers requested a review from LennartPurucker May 14, 2024 07:48
Copy link
Contributor

@LennartPurucker LennartPurucker left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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

@PGijsbers
Copy link
Collaborator

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
Copy link
Contributor

I agree, feel free to merge!

@PGijsbers PGijsbers merged commit 923b49d into openml:develop May 15, 2024
4 of 10 checks passed
@PGijsbers
Copy link
Collaborator

Thanks again @BrunoBelucci 👏

@LennartPurucker
Copy link
Contributor

Thank you @BrunoBelucci!

@BrunoBelucci BrunoBelucci deleted the fix/config_file_being_overwritten branch May 16, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config file being overwritten at import
3 participants