Skip to content

Conversation

@SylvainCorlay
Copy link
Member

Fixes #7769 .
This is not very polished but seems to be doing the job. Will not be able to look more into it before the week end.

@takluyver takluyver added this to the 4.0 milestone Feb 13, 2015
@SylvainCorlay
Copy link
Member Author

Ok, Travis is happy now - ready for review.

@SylvainCorlay SylvainCorlay changed the title Split instance_init in two Do not instantiate traitlet default values if not necessary Feb 17, 2015
@SylvainCorlay
Copy link
Member Author

Rebased to account for the latest changes merged in traitlets, and then simplified a little bit.
(Btw, the test failure in earlier commit 5d2961a does not look legitimate).

@SylvainCorlay
Copy link
Member Author

Just rebased but it seems to make Travis unhappy.

@SylvainCorlay
Copy link
Member Author

The failing test seems unrelated.

@minrk
Copy link
Member

minrk commented Mar 20, 2015

Can you document the new functionality? The methods you replaced were public APIs whose behavior was documented. decorate is completely undocumented (and still unclear to me), as is static_init. Which is called when, and what do they do? Why is instance_init totally removed, rather than changing what behaviors happen at instance init time?

@SylvainCorlay
Copy link
Member Author

Yes, I will add docstrings this afternoon. In short instance_init was doing multiple things

  • instantiating (and therefore validating right away) the default values of the TraitType instance.
  • decorating the TraitType instance
    • using functions such as _resolve_classes
    • or by setting the this_class of the sub-traits for container types.
    • or both

instance_init has been decomposed into two functions: static_init on the one hand which does the first part and decorate on the other hand which does the second part.

The base trait type has an empty decorate function which may be overloaded by other trait types, hence I could remove the test for existence of _resolve_classes for example, which are specific to certain type of traits.

Now, the goal is to deffer the execution of static_init as much as possible. Initially, the constructor of HasTrait was calling instance_init for all traits. Now it only calls static_init at this point only if a value is provided for the considered attribute. (Actually it calls set_default_value which hooks up potential _%s_default functions). In the case where it is not called at this point, no default value has been instantiated. The default value is only created and validated the first time it is accessed line 419

@minrk
Copy link
Member

minrk commented Mar 20, 2015

Great, thanks for the explanation. I think decorate is part of what's tripping me up. decorate means something in Python, and it's not what is going on here. It seems like the basics of finishing initialization once there is an instance associated. Is decorate always called? If so, I might leave it called instance_init, and then separate the value initialization (now in static_init) from the Trait initialization, which I think is the actual goal here. If that's the case, I would probably leave decorate as instance_init, and call static_init something like static_default_value or init_default_value.

@SylvainCorlay
Copy link
Member Author

Done, I just added some documentation and renamed the methods accordingly to your recommendation. Indeed decorate is quite overloaded in Python.

@minrk
Copy link
Member

minrk commented Mar 20, 2015

Terrific, thanks. I think it's a lot easier to follow, now. Will merge when Travis gives the green light.

@minrk
Copy link
Member

minrk commented Mar 21, 2015

The failing tests are real, but due to a slightly pathological behavior in the in-process kernel init.

The in-process kernel asks for traits passed in kwargs before calling super().__init__.

A test case:

from IPython.utils.traitlets import HasTraits, Instance

class C(HasTraits):
    t = Instance(int, allow_none=False)

    def _t_default(self):
        return 5

    def __init__(self, **kwargs):
        print(self.t)
        super().__init__(self, **kwargs)

C() # works, t=5
C(t=2) # Fails with t is None, but allow_none=False
  1. HasTraits.__new__ sees that t is given in kwargs, and thus skips set_default_value, preventing _t_default from being registered as an
  2. C.__init__ requests t, but it hasn't been assigned yet, and _t_default was skipped, by the above, so t.default_value() is called instead, returning None.
  3. super().__init__(**kw) actually assigns c.t from kwargs, as expected.

I don't think it's a problem that you cannot reliably access dynamic trait initialization before calling HasTraits.__init__, since nobody should really be doing that. I can remove the behavior in the in-process kernel without issue, I think.

appears to be unnecessary due to upstream changes,
and no longer works with traitlets refactor.
@minrk
Copy link
Member

minrk commented Mar 21, 2015

I made SylvainCorlay#3 to remove the weird in-process behavior.

@SylvainCorlay
Copy link
Member Author

Thanks, I was just looking again into this. (the tests used to pass before I rebased.)

remove problematic in-process kernel behavior
minrk added a commit that referenced this pull request Mar 21, 2015
Do not instantiate traitlet default values if not necessary
@minrk minrk merged commit 2af3946 into ipython:master Mar 21, 2015
@SylvainCorlay SylvainCorlay deleted the fix_instance_init branch March 21, 2015 04:05
@SylvainCorlay
Copy link
Member Author

Thanks!

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.

Trait default values are validated even when not ever used

3 participants