Initializing the metadata of a trait

[ipython]

Initializing the metadata of a trait

[ssanderson]

👍 for either explicit metadata as a dictionary argument, or for .tag accepting **kwargs

[ellisonbg]

+1 on A1 and B1

On Fri, Jul 17, 2015 at 8:59 AM, Scott Sanderson [email protected]
wrote:

[image: 👍] for either explicit metadata as a dictionary argument, or
for .tag accepting **kwargs

—
Reply to this email directly or view it on GitHub
#52 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[jasongrout]

Splitting my vote, +0.7 on option A1, and +0.3 on B3. A1 is more pythonic. Python does not generally chain methods onto constructors, and usually methods that mutate state (like tag does in this case) would return None, not the object. The thing I don't like about A1 is that it is the most verbose of all the options, which is almost enough for me to vote for B3.

[ssanderson]

There might also be an option A3 using tags to shorten the name but still keeping the explicit argument:

x = Int(allow_none=True, tags={'sync': True})

[SylvainCorlay]

B1 or B3

[sccolbert]

The reason that atom is using tag is because there is a metadata read-write property on the member instance. tag is just a shortcut to set metadata and return self. I'm not a fan of a metadata keyword argument.

[jasongrout]

@ellisonbg - do you know why traitlets has a metadata class member that is used as the default metadata, but the real metadata is stored in ._metadata with get_metadata and set_metadata functions? I've been wondering why we don't just have a metadata attribute that is a dict, with some other way of having default metadata specified in the class.

[takluyver]

Can we consider making a project with a new name for changes like this rather than changing the API of traitlets? I don't relish the idea of having to go through and change most of the files in a dozen or so projects to a revised traitlets API. And starting to plan backwards-incompatible API changes straight after the first release doesn't exactly inspire confidence that it will be a stable API for other people to build on.

[jasongrout]

@takluyver, we have the manpower to change the dozens of files (I just did it for the Int vs. Int() change for another PR). These changes we are discussing are backwards-compatible, with deprecations that will probably be measured in years (at the rate we release things, anyway). What we're wanting to do is provide ways around the warts in traitlets for new code, particularly just before/while other projects are starting to adopt the traitlets library, so that they can write cleaner code.

[ellisonbg]

@jasongrout I don't remember why I implemented metadata that way :(

@takluyver trying to evolve software projects and never have API breakage is simply not realistic for projects that are on the edge of a rapidly changing ecosystem. This is what version numbering is for. I think we should absolutely have deprecation cycles and make this easy on users, but to create forks of our own projects just to a avoid API changes doesn't make sense as most people will switch over to the new, faster moving project. In the end, the same thing can be accomplished with good version numbers.

[ellisonbg]

I would prefer to stick with the historic name metadata in traitlets, even as we evolve the API.

[jasongrout]

Option B1 conflicts with the current .metadata attribute that provides default class-level metadata, and it's the natural name for the actual metadata attribute, so -1 on Option B1.

[jasongrout]

Of the B options, I like B3 the best, since it's short and it's a verb. Another good thing about .tag() is that it doesn't convey that it is the only metadata. Having metadata as a constructor argument seems to indicate that what is passed is the only metadata the trait will have, but in reality we'd be combining (and overriding) the default metadata. I think .tag() more clearly conveys that we are updating the existing metadata dictionary.

[jasongrout]

As for the default metadata, how about this change in behavior (assuming we go with option A1, but similarly for other options):

class MyTraitedClass(HasTraits):
    metadata = {'key': 'value'} # default class-level metadata
    def __init__(self, ..., metadata=None):
        self.metadata = self.metadata.copy()
        if metadata is not None:
            self.metadata.update(metadata)

It would make metadata retrieval much simpler, and also preserve the use of the class member for default metadata. I don't like that it changes the purpose of the .metadata attribute when the class is instantiated by localizing the member, but it would make the change backwards-compatible for probably all real-world use-cases. Of course, we'd deprecate the get_metadata() and set_metadata() functions, preferring direct operations with the metadata dict.

Edit: fixed the update to correctly override defaults.

[ellisonbg]

i am fine with this idea for handling default metadata...

On Fri, Jul 17, 2015 at 10:27 AM, Jason Grout [email protected]
wrote:

As for the default metadata, how about this change in behavior (assuming
we go with option A1, but similarly for other options):

class MyTraitedClass(HasTraits):
metadata = {'key': 'value'} # default class-level metadata
def init(self, ..., metadata=None):
if metadata is None:
metadata = {}
metadata.update(self.metadata)
self.metadata = metadata

It would make metadata retrieval much simpler, and also preserve the use
of the class member for default metadata. I don't like that it changes the
purpose of the .metadata attribute when the class is instantiated by
localizing the member, but it would make the change backwards-compatible
for probably all real-world use-cases. Of course, we'd deprecate the
get_metadata() and set_metadata() functions, preferring direct operations
with the metadata dict.

—
Reply to this email directly or view it on GitHub
#52 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[sccolbert]

@jasongrout I think you have your update arguments reversed. The way you have it, the default metadata would override the user supplied metadata.

[jasongrout]

@sccolbert - right! Fixed.

[SylvainCorlay]

OK given the conflict with the already existing metadata attribute in B1, I vote for B3.
And I would -1 the choice of having a metadata dict keyword argument. It is too long for things such as specifying sync=True in widgets...

[minrk]

Any backward-incompatible changes here are going to have to go through a long (at least a year or two) deprecation cycle.

[jasongrout]

@minrk - yep. I think an exception could be made for making the class-level metadata dict a local one when an instance is created. I'd probably consider that change more of a bugfix than a deliberate change in intended behavior (I sure hope no one was changing class-level metadata defaults by changing the metadata attribute on an instance!).

[minrk]

@jasongrout sure, hidden implementation details are different than the public API. I think an important piece of this is not changing the existing tests that exercise the deprecated patterns, but adding new ones, instead. The existing tests need to keep passing.

[jasongrout]

Yes, I think that's important too, except for changing the existing tests to check for deprecation messages too.

[minrk]

Yup. It's something we are trying to do in the split that we haven't done well in the past - clearer communication of deprecations, and less abrupt transitions to new things across multiple releases. Preserving tests of deprecated APIs is part of keeping us honest.

[ellisonbg]

Seems like this discussion has stalled without a clear consensus.

• We want to improve this API to remove the problem of mixing arguments of the constructor with metadata key/values.
• We need to have a good long deprecation period of the old API.
• It is hard to evolve the API using the name metadata without breaking BW compat.
• Having the new API use the name tag is one way of solving that.
• There are a range of ideas on how to improve the API given these constraints.

I propose that all interested parties hop on a G+ to make the decision on Monday morning (let's say 9am PDT) if we can't find consensus before then.

[ssanderson]

@ellisonbg sounds good. Do you want to create a hangout link for the meeting and post it here?

[SylvainCorlay]

sounds good.

[ellisonbg]

I hate G+, but I think I have done this properly:

https://plus.google.com/events/co224tq70q2ksdu8rfir6pjveq0

On Sat, Jul 18, 2015 at 10:04 AM, Sylvain Corlay [email protected]
wrote:

sounds good.

—
Reply to this email directly or view it on GitHub
#52 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
[email protected] and [email protected]

[ellisonbg]

Hi guys, I have created a section on our meeting hackpad for this meeting:

https://ipython.hackpad.com/2015-Dev-Meetings-Part-1-3YJG5lv2Hws

I think we should limit the meeting to an hour, but we may have time to make decisions on other traitlets API design things. @jasongrout and @SylvainCorlay can you put a list of the decisions you would like us to make at the meeting on the hackpad.

@fperez if you can't make it, can you appoint a BDFL delegate (maybe Min?) to break any lack of clear consensus?

[fperez]

Unfortunately I can't make it, I have to be in SF all day for a meeting...

Indeed, I think @minrk is a perfect BDFL delegate in this case; Min has an extremely good knowledge of how the traitlets code impacts the config machinery throughout our entire codebase, including the management of clusters in IPython.parallel.

Are you OK with that role, Min?

[minrk]

Sure, I can do that.

[fperez]

Excellent, thanks!

[SylvainCorlay]

@jasongrout Should we close this one?

[SylvainCorlay]

This was fixed in #53 and #56.