Import config parser into TOML parser

[mishaker]

Description

When using S3Client() with TOML parser you get this error:

defaults = dict(configuration.get_config().defaults())
AttributeError: 'LuigiTomlParser' object has no attribute 'defaults'

Importing ConfigParser into toml_parser solves the issue.

[dlstadther]

Could you update a TOML test to verify the successful use of a config param from the default section? Just want to be sure that this import still works as you would expect it to in that S3Client, rather than just removing the error without actual impact. Thanks!

[mishaker]

@dlstadther the core of the problem is that TOML doesn't return default config values (the method is not there). I added a test to show the use case. The problem is not S3Client().

[mishaker]

@honnix @Tarrasch sorry to bug you with this message but anyway I can help you with the review of this one? thanks

[honnix]

I don't have good understanding of the configuration code, but this PR looks weird to me. Why would TOML parser depend on configparser? If we want a default method, let's add it, rather than pulling in a full class that is not really supposed to be there.

[mishaker]

@honnix it is the same logic as in the cfg parser. in default config we have:

class LuigiConfigParser(BaseParser, ConfigParser):
    NO_DEFAULT = object()
    enabled = True
    _instance = None
    _config_paths = [
        '/etc/luigi/client.cfg',  # Deprecated old-style global luigi config
        '/etc/luigi/luigi.cfg',
        'client.cfg',  # Deprecated old-style local luigi config
        'luigi.cfg',
    ]
    ...

And in TOML config:

class LuigiTomlParser(BaseParser):
    NO_DEFAULT = object()
    enabled = bool(toml)
    data = dict()
    _instance = None
    _config_paths = [
        '/etc/luigi/luigi.toml',
        'luigi.toml',
    ]
 ....

Somehow ConfigParser was eliminated which causes default values error. I think if we want to be consistent we have to add it to TOML too.

[honnix]

If I understood correctly, TOML and the Python default config (ConfigParser) are two different ways writing config file, and there is no connection between TOML and ConfigParser. The reason LuigiConfigParser and LuigiTomlParser look differently, is purely a design decision: inheritance vs composition.

[bbreton]

Are there plans to finish this fix? This bug is making me unable to use toml config because I use S3Client. I don't see an obvious workaround absent code changes aside from switching to cfg.

[lallea]

@bbreton This is a community-driven project, without a backing open source based business. You can offer to help by improving the PR, creating a better solution, or offer to fund someone to work on Luigi. But you cannot demand that anyone spend their spare time or company time, nor expect any plan or schedule for things to be fixed.

[honnix]

A commented, inheriting from ConfigParser is not an ideal solution.

[honnix]

I have made request for change explicitly, so we can move forward with an alternative solution.

[honnix]

The fix could be as simple as adding:

    def defaults(self):
        return {}

[mishaker]

@honnix don't hesitate to push on this PR. I actually tried your solution but it didn't work (so I forked the project ever since).

[bbreton]

@lallea Sorry, I was unsure if it was just forgotten or if it even needed additional work. My impression was the fix worked, but it was unclear if the method was the best.

If the PR needs additional work I am certainly willing to help fix it.

@bbreton This is a community-driven project, without a backing open source based business. You can offer to help by improving the PR, creating a better solution, or offer to fund someone to work on Luigi. But you cannot demand that anyone spend their spare time or company time, nor expect any plan or schedule for things to be fixed.

[mishaker]

@bbreton this fix works yes. we use it in production. I am still not sure about @honnix argument. if config should be the same but only different format, then why there is one LuigiConfigParser(BaseParser, ConfigParser) inheriting from ConfigParser but not the other one LuigiTomlParser(BaseParser).

[honnix]

@mishaker As I commented before, config parser and toml parser are two different things, so inheritance doesn't really make sense. I'm not sure why adding a defaults methods didn't work for you, but that's what the exception complains.

[stale]

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.