-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
read file in read mode #1338
Conversation
There was a problem hiding this 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.
… boolean after reading it from config_file
@PGijsbers it is done =) |
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? |
There was a problem hiding this 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.
There was a problem hiding this 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.
The tests seem to be failing due to a (test) server error. |
Yes this was deliberate. |
I agree, feel free to merge! |
Thanks again @BrunoBelucci 👏 |
Thank you @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