Fix error with LuigiTomlParser usage in S3Client #3076
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
junhl
commented
Apr 19, 2021
| @@ -61,6 +61,23 @@ def read(self, config_paths): | |||
|
|
|||
| return self.data | |||
|
|
|||
| def defaults(self): | |||
There was a problem hiding this comment.
solution 1: fill lacking functions to LuigiTomlParser. This is just dummy function so it's not my preferred one.
(will add tests if preferred by reviewers)
junhl
commented
Apr 19, 2021
| try: | ||
| config = dict(configuration.get_config().items('s3')) | ||
| config = dict(luigi_config['s3']) |
There was a problem hiding this comment.
solution 2: change usage to generic one to use existing __getitem__ and only take defaults when it's meaningful (ex. LuigiConfigParser) instead of creating dummy function. (my pref. Will add tests if reviewers also think so)
junhl
changed the title
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
My attempt to address issue of toml usage error in S3Client from #2996
I'm providing two solutions to choose from, one to fill lacking functions into LuigiTomlParser and another to change the usage into more generic one. While I prefer the second one, when the reviewers prefer one over another, I'll add tests for it.
LuigiConfigParser is built on top of Python's ConfigParser, which is MutableMapping at the end.
LuigiTomlParser is using toml (the module), and it does not provide a comparable class but rather provide load/dump via decoder/encoder.
They work a bit differently. A touch to BaseParser may be needed on a long term because there may be other issues due to differences in their functionalities (not being addressed in this PR).
Motivation and Context
Resolves error on S3Client when attempting to use toml. It would give this before:
Have you tested this? If so, how?
I ran my jobs with this code and it works for me.
I'm happy to add tests when one solution is preferred over another by the reviewers.