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

[API change] New way to create command line options. #572

Merged
merged 23 commits into from
Sep 7, 2019

Conversation

gabrieldemarmiesse
Copy link
Collaborator

@gabrieldemarmiesse gabrieldemarmiesse commented Aug 6, 2019

This is backward compatible.

Old command line options:

class DebugOption(CommandLineOption):
    """
    Suppress warnings about missing observers and don't filter the stacktrace.

    Also enables usage with ipython --pdb.
    """

    @classmethod
    def apply(cls, args, run):
        """Set this run to debug mode."""
        run.debug = True


class LoglevelOption(CommandLineOption):
    """Adjust the loglevel."""

    arg = 'LEVEL'
    arg_description = 'Loglevel either as 0 - 50 or as string: DEBUG(10), ' \
                      'INFO(20), WARNING(30), ERROR(40), CRITICAL(50)'

    @classmethod
    def apply(cls, args, run):
        """Adjust the loglevel of the root-logger of this run."""

        try:
            lvl = int(args)
        except ValueError:
            lvl = args
        run.root_logger.setLevel(lvl)

New way (borrowed from the click API, intuitive for people who have used it):

@cli_option('-d', '--debug', is_flag=True)
def debug_option(args, run):
    """
    Set this run to debug mode.
    Suppress warnings about missing observers and don't filter the stacktrace.
    Also enables usage with ipython --pdb.
    """
    run.debug = True


@cli_option('-l', '--loglevel')
def loglevel_option(args, run):
    """
    Loglevel either as 0 - 50 or as string: DEBUG(10),
    INFO(20), WARNING(30), ERROR(40), CRITICAL(50)
    """

    try:
        lvl = int(args)
    except ValueError:
        lvl = args
    run.root_logger.setLevel(lvl)

ex = Experiment('My_exp', additional_cli_options=[debug_option, loglevel_option]

@gabrieldemarmiesse gabrieldemarmiesse marked this pull request as ready for review August 6, 2019 15:32
@coveralls
Copy link

coveralls commented Aug 6, 2019

Coverage Status

Coverage increased (+0.09%) to 85.393% when pulling 0d5a95f on gabrieldemarmiesse:new_command_line_experience into d3fe102 on IDSIA:master.

sacred/experiment.py Outdated Show resolved Hide resolved
@JarnoRFB
Copy link
Collaborator

looks good to me together with #569. From my side we could agree on making this the new default api.

Co-Authored-By: Rüdiger Busche <ruedigerbusche@web.de>
@gabrieldemarmiesse
Copy link
Collaborator Author

@JarnoRFB Cool, I'm waiting a green light from @Qwlouse and then I'll update the docs.

@Qwlouse
Copy link
Collaborator

Qwlouse commented Aug 14, 2019

Interface looks good to me. The only thing I am unsure about is how to handle default commandline options. Currently you are using gather_commandline_options() to provide the defaults and you have a hardcoded list of options there. That makes it difficult both to extend the default options (with say a plugin to sacred), and to remove options from the user side. How about using a global list of default commandline options instead? (Only for the default options. User options would still be passed to the experiment).

@gabrieldemarmiesse
Copy link
Collaborator Author

@Qwlouse good point. Let me change that.

@gabrieldemarmiesse
Copy link
Collaborator Author

@Qwlouse I added DEFAULT_COMMAND_LINE_OPTIONS as per your recommendation.

@gabrieldemarmiesse
Copy link
Collaborator Author

@Qwlouse Are you ok with the new implementation?

Copy link
Collaborator

@Qwlouse Qwlouse left a comment

Choose a reason for hiding this comment

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

Looks very good, but I would add some validation to the CLI options to catch errors early and provide an informative error message.

sacred/commandline_options.py Show resolved Hide resolved
@gabrieldemarmiesse
Copy link
Collaborator Author

I added the correctness checks. One last look maybe before we can merge?

@Qwlouse Qwlouse merged commit e9fe260 into IDSIA:master Sep 7, 2019
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.

4 participants