Skip to content

generate all config, not just new traits on each class#385

Merged
minrk merged 7 commits intoipython:masterfrom
minrk:not-own-traits
Apr 11, 2017
Merged

generate all config, not just new traits on each class#385
minrk merged 7 commits intoipython:masterfrom
minrk:not-own-traits

Conversation

@minrk
Copy link
Copy Markdown
Member

@minrk minrk commented Mar 24, 2017

using class_own_traits splits up config for classes with parents, and hides some config options entirely if traits are defined in base classes not in the final classes list.

The change has not been released, so this restores the behavior of all released traitlets versions.

@ankostis
Copy link
Copy Markdown
Contributor

ankostis commented Mar 24, 2017

using class_own_traits splits up config for classes with parents,

Yes, that is intentional by #289, to deduce clutter when generating help and sample-config files with non-trivial hierarchies of Configurables. And to compensate the "split", that PR had added the base-classes in the output, to help user decide in which class to set a config-param.

To give an example of why I had crafted that PR, in our application , it reduced the number of lines of the generated sample configuration file by a factor of x5:

$ wc -l config-PR385.py config-PR289.py
  3578 config-PR385.py
   729 config-PR289.py

The extra lines were all identical copies of help-messages from traits included already in subclasses.

So would it be possible to identify the root problem instead of going back to old behavior?

and hides some config options entirely if traits are defined in base classes not in the final classes list.

If that is happening, that's a certain bug: adding a subclass in the classes list should be enough for all its baseclass traits to appear somewhere in the help-messages.

@ankostis
Copy link
Copy Markdown
Contributor

A possible fix would be not to include the help-message again on each subclass but just refere to the baseclass?

@minrk
Copy link
Copy Markdown
Member Author

minrk commented Mar 27, 2017

I realize it was intentional, but I discovered after using it that the result is a significant regression.

So would it be possible to identify the root problem instead of going back to old behavior?

When generating config for an object, I want to see everything I can configure on that object in one place. Relying on references and inheritance means that I have to look in several different disconnected places in the file in order to see the configuration of one object. So the important thing for me is that when I see the section of a config file corresponding to one object, I get every configurable for that object.

Since this is a regression from previous releases, perhaps we could make the 'only not inherited' behavior an off-by-default option?

@ankostis
Copy link
Copy Markdown
Contributor

Certainly adding a flag is doable, but maybe the following 2 alternatives improve the UI experience:

a. Not repeating help messages on each config-param copy.
b. Collecting all sub-class parameters beneath the 1st metnion of the base-class param.

If you think it is worth it, instead of a flag, I can craft some mockups here for config-files/help msgs to compare and decide, should I?

@minrk
Copy link
Copy Markdown
Member Author

minrk commented Mar 27, 2017

If you think it is worth it, instead of a flag, I can craft some mockups here for config-files/help msgs to compare and decide, should I?

sure, that would be great!

The most important thing for me is that when I see a class's entry in a config file, I see every configurable trait for that class. There does need to be some explanation of it, at least. How's this for a compromise:

  • always show all configurable traits for every class
  • only show full description on defining class
  • on subclasses, show first line of help and originating Class.trait name for easy reference to original
  • Fix bug: ensure defining class is always included in the config file

@minrk
Copy link
Copy Markdown
Member Author

minrk commented Mar 27, 2017

Here's the pattern I found that prompted this PR:

A collection of configurable classes with the same/similar config options inherited from a Mixin. That Mixin is intended to be an implementation detail, and shouldn't affect the config file. In a typical config file, only one of these classes will be configured, so the norm is to find the section corresponding to your chosen class and configure it. In master, all of these classes produce empty config sections, which is not a good experience, and the Mixin defining the configurable traits isn't present, so it is impossible to discover what is configurable.

The best experience in these cases would be for every class in the classes list to get all of its config with all help in one place. It would be preferable to avoid including the Mixin class in the config file, and it would be preferable to avoid having 'references' to the originating classes, which are excluded from classes on purpose.

So something like this should get the right result:

  • identify 'defining' class for a given trait as a class whose parent is not in the classes list
  • truncate help to one line + "See Original.trait..." if the defining class is elsewhere in the config file

using class_own_traits splits up config for classes with parents,
and hides some config options entirely if traits are defined
in base classes not in the final `classes` list.

The change has not been released,
so this restores the behavior of all released traitlets versions.
@minrk minrk added this to the 5.0 milestone Mar 27, 2017
@minrk
Copy link
Copy Markdown
Member Author

minrk commented Mar 27, 2017

I've updated this PR to reflect the last proposal - a class always shows its configurable traits. The help is truncated to the first line plus "See also" if its parent class is also in the config file.

The implicit inclusion of all parent classes is also removed, because it is no longer needed to ensure that all the config options show up somewhere. The implicit behavior effectively changed the meaning of the classes list from the list of classes that should be considered for inclusion to the list of classes to walk, including all of their parents, making it impossible for authors to request that a parent class be excluded from config.

The behavior is now much closer to the current release behavior, with the exception that redundant help output is truncated.

@ankostis
Copy link
Copy Markdown
Contributor

ankostis commented Mar 27, 2017

Given this class diagram:

HasTraits <--+-- Irrelevant  ## No configurable trait defined
             |
             +-- TT1 <-------- T1
             |    +--tt1_a      +--t1_b
             |
             +-- T2 <----+
                  +t2_a  |
                  +t2_b  |
                         |
Application <------------+----> App
                                 +--t2_a   ##Override existing trait
                                 +--classes = [T1]

I'm trying to run invoke A.generate_config_file() with this PR and I'm getting:

AttributeError: type object 'T1' has no attribute 'class_config_section'` 

edit: pasting the "rough" code for the above diagram:

import traitlets.config as trtc
import traitlets as trt


Base = trtc.Configurable


class _Irrelevant(Base):
    pass


class TT1(Base):
    tt1_a = trt.Bool(help="GRANDMOTHER HELP", config=True)


class T1(TT1):
    t1_b = trt.Bool(help="Multi-line help message.\n\nLorem ipsum dolor sit amet, consectetur adipiscing elit. "
                    "Quisque eu nunc cursus, aliquam ante at, porttitor eros. Vestibulum et dignissim erat. "
                    "Suspendisse tempor, est eget pretium mollis, nulla libero eleifend ante, "
                    "eu tristique est quam sed ligula. Phasellus ac odio nunc. "
                    "Phasellus gravida ornare lorem, hendrerit venenatis velit facilisis mollis. ",
                    config=True)


class T2(Base):
    t2_a = trt.Bool(help="Instructios for destruction.", config=True)
    t2_b = trt.Bool(help="Overriden help-msg.\n\nAnd is now a bit longer, at least...", config=True)


class A(trtc.Application, T2, _Irrelevant):
    classes = [T1, ]
    t2_a = trt.Bool(help="Simple verbose flag.", config=True)


a = A()

print(a.generate_config_file())

@ankostis
Copy link
Copy Markdown
Contributor

(pasted the code for my previous comment)

@ankostis
Copy link
Copy Markdown
Contributor

@minrk is this PR (738ce4d) supposed to work correctly, or rather WIP?
If I replace HasTraits with Configurable, it runs ok, but the output is missing most of the base-classes:

# Configuration file for application.

#------------------------------------------------------------------------------
# A configuration
#------------------------------------------------------------------------------
## This is an application.

## The date format used by logging formatters for %(asctime)s
    #  Default: '%Y-%m-%d %H:%M:%S'
# c.A.log_datefmt = '%Y-%m-%d %H:%M:%S'

## The Logging format template
    #  Default: '[%(name)s]%(highlevel)s %(message)s'
# c.A.log_format = '[%(name)s]%(highlevel)s %(message)s'

## Set the log level by value or name.
#  Choices: (0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL')
    #  Default: 30
# c.A.log_level = 30

## Simple verbose flag.
    #  Default: False
# c.A.t2_a = False

## Overriden help-msg.
#  
#  And is now a bit longer, at least...
    #  Default: False
# c.A.t2_b = False

#------------------------------------------------------------------------------
# T1 configuration
#------------------------------------------------------------------------------
## Multi-line help message.
#  
#  Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque eu nunc
#  cursus, aliquam ante at, porttitor eros. Vestibulum et dignissim erat.
#  Suspendisse tempor, est eget pretium mollis, nulla libero eleifend ante, eu
#  tristique est quam sed ligula. Phasellus ac odio nunc. Phasellus gravida
#  ornare lorem, hendrerit venenatis velit facilisis mollis.
    #  Default: False
# c.T1.t1_b = False

## GRANDMOTHER HELP
    #  Default: False
# c.T1.tt1_a = False

@ankostis
Copy link
Copy Markdown
Contributor

ankostis commented Mar 27, 2017

master(c622c8e): Missing base-classes bug

In master, all of these classes produce empty config sections, which is not a good experience, and the Mixin defining the configurable traits isn't present, ...

Strangely, i still cannot reproduce that bug you describe also in OP.

For the above class-hierarchy, master lists all, and only all, configurables that have config==True traits. Check the printout in the next section.

@minrk what am I missing here?

master: Printout of configurables on master

If I understand correctly what you want, the only thing missing from the printout below is to list all config-traits on each class, with a "see above" reference., correct?

# Configuration file for application.

#------------------------------------------------------------------------------
# TT1(Configurable) configuration
#------------------------------------------------------------------------------
## GRANDMOTHER HELP
#  Default: False
#c.TT1.tt1_a = False

#------------------------------------------------------------------------------
# T1(TT1,Irrelevant) configuration
#------------------------------------------------------------------------------
## Multi-line help message.
#  
#  Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque eu nunc
#  cursus, aliquam ante at, porttitor eros. Vestibulum et dignissim erat.
#  Suspendisse tempor, est eget pretium mollis, nulla libero eleifend ante, eu
#  tristique est quam sed ligula. Phasellus ac odio nunc. Phasellus gravida
#  ornare lorem, hendrerit venenatis velit facilisis mollis.
#  Default: False
#c.T1.t1_b = False

#------------------------------------------------------------------------------
# T2(Configurable) configuration
#------------------------------------------------------------------------------
## Instructios for destruction.
#  Default: False
#c.T2.t2_a = False

## Overriden help-msg.
#  
#  And is now a bit longer, at least...
#  Default: False
#c.T2.t2_b = False

#------------------------------------------------------------------------------
# Application(SingletonConfigurable) configuration
#------------------------------------------------------------------------------
## This is an application.

## The date format used by logging formatters for %(asctime)s
#  Default: '%Y-%m-%d %H:%M:%S'
#c.Application.log_datefmt = '%Y-%m-%d %H:%M:%S'

## The Logging format template
#  Default: '[%(name)s]%(highlevel)s %(message)s'
#c.Application.log_format = '[%(name)s]%(highlevel)s %(message)s'

## Set the log level by value or name.
#  Choices: (0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL')
#  Default: 30
#c.Application.log_level = 30

#------------------------------------------------------------------------------
# A(Application,T2,Irrelevant) configuration
#------------------------------------------------------------------------------
## This is an application.

## Simple verbose flag.
#  Default: False
#c.A.t2_a = False

This PR fails if baseclass is a HasTraits

Just because I had forgotten that we use only Configurable subclasses, not HasTraits, I discovered by accident the following bug with this PR (738ce4d):

I'm trying to run invoke A.generate_config_file() with this PR and I'm getting: AttributeError: type object 'T1' has no attribute 'class_config_section'

Anyhow, the code in master does not fail, which I believe is the correct behavior.
Unfortunately, TCs did not catch this case, so I can add the class-hierarchy as extra TC.
Should I?

Why adding Application as first element in self.classes?

Finally, a tale from the "crypt": on Application init, self is inserted into classes if not already there, in position zero; but eventually classes are printed in reversed-mro.
Do you have any idea why it is not append Application.self at the end?

EDIT:

Just understood that you move into a different direction, making classes being authoritative list. Not sure everything I said above still applies.

Comment thread traitlets/config/application.py Outdated

if classes:
help_classes = self._classes_with_config_traits()
help_classes = self.classes
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should respect the classes present in Application.classes, but we cannot hide classes that are relevant when applying config-values.
Actually, _classes_with_config_traits() reads this list, and augments it with any relevant baseclasses.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, now I saw the change in loader below, so you really mean that :-)
But isn't that a breaking change?

@ankostis
Copy link
Copy Markdown
Contributor

ankostis commented Mar 27, 2017

The implicit inclusion of all parent classes is also removed, because it is no longer needed to ensure that all the config options show up somewhere.
...
The behavior is now much closer to the current release behavior, with the exception that redundant help output is truncated.

Although this is a bold step that have its merit (i.e. produce real terse results & see below), I don't think it is preserving released 4.3.2 behavior.

For instance, this program fails to print C base class, although it is used when applying configurations:

import traitlets.config as trtc
import traitlets as trt

class C(trtc.Configurable):
    a = trt.Bool(help='1st help line.\n\nOther lines.').tag(config=True)

class A(trtc.Application, C):
    pass

print(A().generate_config_file())

c=trtc.get_config()

## Set trait on base-class.
c.C.a = True
print("A.a: ", A(config=c).a)

v4.3.2:

# Configuration file for application.

#------------------------------------------------------------------------------
# C(Configurable) configuration
#------------------------------------------------------------------------------

## 1st help line.
#
#  Other lines.
#c.C.a = False

#------------------------------------------------------------------------------
# Application(SingletonConfigurable) configuration
#------------------------------------------------------------------------------

## This is an application.

## The date format used by logging formatters for %(asctime)s
#c.Application.log_datefmt = '%Y-%m-%d %H:%M:%S'

## The Logging format template
#c.Application.log_format = '[%(name)s]%(highlevel)s %(message)s'

## Set the log level by value or name.
#c.Application.log_level = 30

#------------------------------------------------------------------------------
# A(Application,C) configuration
#------------------------------------------------------------------------------

## This is an application.

A.a:  True

This PR(738ce4d):

# Configuration file for application.

#------------------------------------------------------------------------------
# A configuration
#------------------------------------------------------------------------------
## This is an application.

## 1st help line.
#
#  Other lines.
    #  Default: False
# c.A.a = False

## The date format used by logging formatters for %(asctime)s
    #  Default: '%Y-%m-%d %H:%M:%S'
# c.A.log_datefmt = '%Y-%m-%d %H:%M:%S'

## The Logging format template
    #  Default: '[%(name)s]%(highlevel)s %(message)s'
# c.A.log_format = '[%(name)s]%(highlevel)s %(message)s'

## Set the log level by value or name.
#  Choices: (0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL')
    #  Default: 30
# c.A.log_level = 30

A.a:  True  @@@ Config-values on `C` class still apply, although not included in help message.

master + minor fixes(1fe0d48):

# Configuration file for application.

#------------------------------------------------------------------------------
# C(Configurable) configuration
#------------------------------------------------------------------------------
## 1st help line.
#
#  Other lines.
#  Default: False
#c.C.a = False

#------------------------------------------------------------------------------
# Application(SingletonConfigurable) configuration
#------------------------------------------------------------------------------
## This is an application.

## The date format used by logging formatters for %(asctime)s
#  Default: '%Y-%m-%d %H:%M:%S'
#c.Application.log_datefmt = '%Y-%m-%d %H:%M:%S'

## The Logging format template
#  Default: '[%(name)s]%(highlevel)s %(message)s'
#c.Application.log_format = '[%(name)s]%(highlevel)s %(message)s'

## Set the log level by value or name.
#  Choices: (0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL')
#  Default: 30
#c.Application.log_level = 30

#------------------------------------------------------------------------------
# A(Application, C) configuration
#------------------------------------------------------------------------------
## This is an application.

A.a:  True

Assuming that this bug is fixed, it might make sense to introduce a new flag (e.g. classes_list_is_authoritative) to enable it as a new behavior. But I would not make it the default, because, besides breaking old behavior, in complex hierarchies it requires a lot of repetition to manually maintain classes-list to achieve the same flexibility and easiness.

@ankostis
Copy link
Copy Markdown
Contributor

@minrk how about printing all the parent parameters at the end of each class, like this(1e2c6cb)?

# Configuration file for application.

#------------------------------------------------------------------------------
# C(Configurable) configuration
#------------------------------------------------------------------------------
## 1st help line.
#
#  Other lines.
#  Default: False
#c.C.a = False

#------------------------------------------------------------------------------
# Application(SingletonConfigurable) configuration
#------------------------------------------------------------------------------
## This is an application.

## The date format used by logging formatters for %(asctime)s
#  Default: '%Y-%m-%d %H:%M:%S'
#c.Application.log_datefmt = '%Y-%m-%d %H:%M:%S'

## The Logging format template
#  Default: '[%(name)s]%(highlevel)s %(message)s'
#c.Application.log_format = '[%(name)s]%(highlevel)s %(message)s'

## Set the log level by value or name.
#  Choices: (0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL')
#  Default: 30
#c.Application.log_level = 30

#------------------------------------------------------------------------------
# A(Application, C) configuration
#------------------------------------------------------------------------------
## This is an application.


## Parameters from parent classes:
#
#c.A.a = False
#c.A.log_datefmt = '%Y-%m-%d %H:%M:%S'
#c.A.log_format = '[%(name)s]%(highlevel)s %(message)s'
#c.A.log_level = 30
A.a:  True

minrk added 6 commits March 28, 2017 10:57
was being added to the class attr instead of the instance attr,
having no practical effect.
exclude 'Choices' and 'Default' on repeated entries
@minrk
Copy link
Copy Markdown
Member Author

minrk commented Mar 28, 2017

I got sidetracked on the class list, that was a mistake. The list of classes should now actually be unchanged from 4.x and master.

how about printing all the parent parameters at the end of each class, like this(1e2c6cb)?

That's almost there, as long as there is at least some help for every inherited trait. Showing no help and not where it came from isn't enough.

This PR now meets my basic requirement:

  • every class shows all configurable traits with some help

and should have some benefit from the changes for brevity:

  • Inherited traits where the defining class is in the config file show only the first line of help and a reference to the defining parent

# Truncate help to first line + "See also Original.trait"
if trait.help:
lines.append(c(trait.help.split('\n', 1)[0]))
lines.append('# See also %s.%s' % (defining_class.__name__, name))
Copy link
Copy Markdown
Contributor

@ankostis ankostis Mar 28, 2017

Choose a reason for hiding this comment

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

I would put a : at the end of the label to look like a "field", like "# Default:" and "# Choices:".

edit: Or just "# See: ...".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, fixed.

@ankostis
Copy link
Copy Markdown
Contributor

ankostis commented Mar 28, 2017

This PR now meets my basic requirement:

Mine too. Thanks.

Have you also retrofitted the help-message & RsT doc generation?

@minrk
Copy link
Copy Markdown
Member Author

minrk commented Mar 28, 2017

Have you also retrofitted the help-message & RsT doc generation?

I haven't, I've left them alone. I might prefer to keep this PR confined to config-file output.

@minrk minrk merged commit 0aa4350 into ipython:master Apr 11, 2017
@minrk minrk deleted the not-own-traits branch April 11, 2017 13:34
dvr = trait.default_value_repr()
lines.append(indent('# Default: %s' % dvr, 4))
lines.append('#c.%s.%s = %s' % (cls.__name__, name, dvr))
lines.append('# c.%s.%s = %s' % (cls.__name__, name, default_repr))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You added a space after # on the command; is that on purpose?
I had put no space there to be easy to spot which lines are comments and which comments,
and to comment/uncomment a line with a single keystroke.
[edit] review started by mistake.

# section header
breaker = '#' + '-'*78
parent_classes = ','.join(p.__name__ for p in cls.__bases__)
breaker = '#' + '-' * 78
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

breaker = '##' + '-' * 76

To separate class headers even better.

if issubclass(p, Configurable)
)

s = "# %s(%s) configuration" % (cls.__name__, parent_classes)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s = "## %s(%s) configuration" % (cls.__name__, parent_classes)

Same as above.

@ankostis
Copy link
Copy Markdown
Contributor

Apologize for being so late here.
I had written these a month ago as "review" items by mistake (instead of plain comments),
but now I realized that nobody else could view my comments until now, after I closed & submitted the review.

ankostis added a commit to ankostis/traitlets that referenced this pull request Apr 13, 2017
ankostis added a commit to ankostis/traitlets that referenced this pull request Apr 13, 2017
Difference:

PREVIOUSLY:
#------------------------------------------------------------------------------
# A(Application, C) configuration
#------------------------------------------------------------------------------
## This is an application.

## 1st help line.
#  See also C.a
# c.A.a = traitlets.Undefined


THIS PR:
##------------------------------------------------------------------------------
## A(Application, C) configuration
##------------------------------------------------------------------------------
## This is an application.

## 1st help line.
#  See also: C.a
#c.A.a = traitlets.Undefined
minrk added a commit that referenced this pull request May 5, 2017
style(cfg): apply review items in #385 for gen-config
@Carreau Carreau added 5.0-re-review Need to re-review for potential API impact changes. 5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc) and removed 5.0-re-review Need to re-review for potential API impact changes. labels Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5.0-no-incidence change that has noincidence on 5.0 compat (eg: doc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants