Fix 6341 disallow session config in fromparent #6387

RonnyPfannschmidt · 2019-12-31T20:25:01Z

Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

bluetech · 2020-01-01T13:19:59Z

src/_pytest/nodes.py

@@ -144,8 +144,10 @@ def __init__(
                self._nodeid += "::" + self.name

    @classmethod
-    def from_parent(cls, parent, *, name):
-        return cls._create(parent=parent, name=name)
+    def from_parent(cls, parent, **kw):


Can you explain this change? The previous signature already didn't allow config and session, why change to **kw here?

this is to allow super().from_parent - else subclasses would have to use horrendous hacks around the internal '_create' method

FWIW, I still think we should not have / need from_parent to begin with, but could use raising TypeError and/or type annotations. See #5975 (comment). I've hoped there to go back to "the simpler pattern" before that PR was merged even.

@blueyed from_parent will very likely stay in order to manage the logic parts of node construction

its very likely that Node.from_parent will eventually manage passing config, session, parent and nodeid to the Node constructor - in which case it will stay forever

IIUC, adding from_parent is needed to allow for a deprecation of the current way of constructing Node and subclasses.

Again IIUC, the current Node constructor (and subclasses) allow for the creation of broken states and configuration, for example Function and Session objects with different Configs, so the idea is for __init__ to no longer be public API, used only internally.

Perhaps it would help if @blueyed provided a code sample of what the Node.__init__ method would looke like in his mind?

Perhaps it would help if @blueyed provided a code sample of what the Node.__init__ method would looke like in his mind?

It could behave as from_parent if only parent is provided, and ensure that no other (conflicting) kwargs are there (raising a DeprecationWarning (and later TypeError) then, e.g. for when config or session is passed also).

My main issue here is that it complicates things in general, and requires plugins to adopt it (adding a compat layer there), e.g. pytest-asyncio (https://github.com/pytest-dev/pytest-asyncio/blob/6f72dd9f419f7945906d566a85ba8d50b8d8324d/pytest_asyncio/plugin.py#L39).

IIUC, adding from_parent is needed to allow for a deprecation of the current way of constructing Node and subclasses.

Well, the deprecation is for having to use from_parent after all.
From what I can tell we could also have deprecations via __init__ for when an old/unsupported combination/pattern is used.

for me a key problem is construction correctness, right now there are just too many things entangled that cant be easily be de-tangled with a constructor without keeping unreasonably smart __init__ methods -- imho the unreasonable smartness needs to be dissolved or moved into factory functions/methods (like from_parent)

i have been fighting with the details over a long time now as i tried repeatedly to make FunctionDefinition an actual part of the collection tree, and it just keeps breaking and breaking in strange places

my goal is to remove those breakage points one by one so i c an put things in place, one of those stumbling blocks are unreasonably smart constructors (they just do way too much)

i'm happy to go for a different solution that works, but so far nobody has show one, as such im slowly clipping away at the things that make future work utterly horrible for me,

as for the constructors as they currently work - im not aware of a way to untangle them without making them inaccessible at once to allow horrendous internal breakages that wont affect users

also sorry for the late reply, i missed the comment earlier, @nicoddemus brought it to my attention

@RonnyPfannschmidt I can see that it makes sense to get there, but it is bad when plugins have to add compat code for it then, i.e. it would be best to remove it again before releasing it.

As for getting there with type annotations and in small steps see e.g. #6461.

@blueyed its very likely going to stay (so the __init__ wont have to have logic) and i expect it to take many months to completely sort things out

changelog/6341.bugfix.rst

changelog/6387.feature.rst

nicoddemus · 2020-01-03T23:44:25Z

src/_pytest/nodes.py

@@ -144,8 +144,10 @@ def __init__(
                self._nodeid += "::" + self.name

    @classmethod
-    def from_parent(cls, parent, *, name):
-        return cls._create(parent=parent, name=name)
+    def from_parent(cls, parent, **kw):


IIUC, adding from_parent is needed to allow for a deprecation of the current way of constructing Node and subclasses.

Again IIUC, the current Node constructor (and subclasses) allow for the creation of broken states and configuration, for example Function and Session objects with different Configs, so the idea is for __init__ to no longer be public API, used only internally.

Perhaps it would help if @blueyed provided a code sample of what the Node.__init__ method would looke like in his mind?

src/_pytest/nodes.py

changelog/6341.bugfix.rst

changelog/6387.feature.rst

nicoddemus · 2020-01-04T13:02:36Z

src/_pytest/nodes.py

@@ -144,8 +144,10 @@ def __init__(
                self._nodeid += "::" + self.name

    @classmethod
-    def from_parent(cls, parent, *, name):
-        return cls._create(parent=parent, name=name)
+    def from_parent(cls, parent, **kw):


I noticed that none of Node.from_parent has docstrings or type annotations. Missing docstrings also mean they don't appear in the reference doc.

I believe we should explain here how subclasses are meant to use super() that's mentioned in the changelog entry.

I think this is valid for all from_parent methods, but it is absolutely crucial for Node.from_parent: users will be drawn to this method after seeing the warning when Nodes are created via __init__, so this should really be a great docstring IMO. 😁

RonnyPfannschmidt · 2020-01-09T21:00:03Z

put it into wip as i didn't get to clean-up for personal reasons, currently i'm considering it as part of my weekend to-do as i don't think i get to it tomorrow either

nicoddemus · 2020-01-09T21:56:28Z

No worries!

changelog/6387.feature.rst

RonnyPfannschmidt

.

nicoddemus · 2020-01-21T20:31:55Z

@blueyed @bluetech anything else here or can we merge?

blueyed · 2020-01-21T20:35:31Z

changelog/5975.deprecation.rst

@@ -4,3 +4,7 @@ Instead they are new constructed via ``Node.from_parent``.

 This transitional mechanism enables us to detangle the very intensely
 entangled ``Node`` relationships by enforcing more controlled creation/configruation patterns.
+
+As part of that session/config are already disallowed parameters and as we work on the details we might need disallow a few more as well.


Suggested change

As part of that session/config are already disallowed parameters and as we work on the details we might need disallow a few more as well.

As part of that session/config are already disallowed parameters and as we work on the details we might need to disallow a few more as well.

This grammar fix was not included.

blueyed

Should have type hints for return types, and signatures.

blueyed · 2020-01-21T20:41:22Z

anything else here or can we merge?

I'm still in the boat that the whole Node.from_parent things taints features unnecessarily, and really still hope to not having to see an actual release where this gets enforced.

Therefore I suggest to rather branch it off into an actual "feature branch" to get it sorted out there, or to not do any releases from features until it can be removed again.
The first option would mean to revert what has been done in this regard already on features, and with the second option it could stay there, but we would start making releases from master (and release branches) like it was planned on the mailing list - in this case other features could be merged into master separately.

nicoddemus · 2020-01-21T21:30:29Z

I'm still in the boat that the whole Node.from_parent things taints features unnecessarily, and really still hope to not having to see an actual release where this gets enforced.

Therefore I suggest to rather branch it off into an actual "feature branch" to get it sorted out there, or to not do any releases from features until it can be removed again.

We can settle on waiting on releasing off features, but how can we ensure we don't stay like this forever?

I have 3 suggestions:

Perhaps @blueyed can you provide a working example where you can remove from_parent and still limit the parameters passed to __init__ (as is the original intention of from_parent)? I think that the only dissent about this approach was that it was hard/difficult to do that technically.
Somebody steps up and reverts the from_parent from features and move this off to a separate branch, as you suggest, so we can move with the 5.4 release soon(ish).
Changing the release process to always release from master is a possibility too, somebody needs to then step up to write rationale, update docs and scripts, and also pick which features from features to merge into master, similar to option 2) because features as it stands now cannot be released.

As it stands we are in the middle of things, with from_parent implemented but not documented, and @blueyed also thinks the approach is unnecessary/harmful in the long term. We need to reach a decision to this impass.

RonnyPfannschmidt · 2020-01-21T21:41:11Z

@nicoddemus from_parent is also the approach to detangleing node construction and the node tree i have taken after repeatedly failing it before

currently i dont think i can sort out constructor entanglement/controll flow sanely without detaching users from the api

nicoddemus · 2020-01-21T21:46:33Z

currently i dont think i can sort out constructor entanglement/controll flow sanely without detaching users from the api

I'm aware, but @blueyed is unconvinced and wants this to be sorted out by using __init__ only. As it stands we are in a deadlock of sorts, so I'm laying out proposals to unlock the deadlock somehow that works for everyone involved (see my 3 proposals above).

RonnyPfannschmidt · 2020-01-21T21:59:29Z

its not just about those 2 parameters, its about iteratively landing a required major refactoring that rearranges entanglements in the Node Tree and constructors within it

from_parent is an attempt to externalize this in a controlled manner that can keep much better compatibility forward

right now i don't believe there is a sustainable way to deliver that set of refactoring in sane manner with just deprecation and time (unless we are ok with doing a breaking release every 1-2 months for a while as things sort out iteratively)

nicoddemus · 2020-01-21T22:07:38Z

Btw: the from_parent topic has been discussed in the ML some months back, so it is a bit of a bummer to have this being "blocked" now.

blueyed · 2020-01-22T18:16:06Z

@nicoddemus

Btw: the from_parent topic has been discussed in the ML some months back, so it is a bit of a bummer to have this being "blocked" now.

I think you mean https://mail.python.org/pipermail/pytest-dev/2019-October/004841.html (from 2019-10-24) then, which was not really a discussion, but only a reference to the PR (#5975 (from 2019-10-16)).

I gave the same feedback there back then (#5975 (comment)), but it was merged nonetheless (by you).

As for your suggestions: I've started some WIP on reverting it myself in blueyed#143, but have not looked into it a while. But it is blocking myself from merging features into my fork.

I think it would make sense for @RonnyPfannschmidt to keep on working on it given the current pattern(s) he wants/has to use, but do so in a separate branch.

Given that the consent from https://mail.python.org/pipermail/pytest-dev/2019-November/004860.html however has been to use master and release branches we could also just go with option 3, which appears to be the long-term goal anyway.

Given all that I am not holding anything back in this regard, and have only repeated myself because you've asked.

blueyed · 2020-01-22T18:17:16Z

But my review comment still applies: having all those from_parent indirections results also in missing type annotations in general, and therefore those wrappers should really have them.

nicoddemus · 2020-01-22T19:26:05Z

I gave the same feedback there back then (#5975 (comment)), but it was merged nonetheless (by you).

That discussion seemed resolved to me, you asked something, @RonnyPfannschmidt replied, and that was that.

Given that the consent from mail.python.org/pipermail/pytest-dev/2019-November/004860.html however has been to use master and release branches we could also just go with option 3, which appears to be the long-term goal anyway.

I don't think it is good practice to change our workflow without having the proper docs in place:

CONTRIBUTING.rst
HOWTORELEASE.rst
PR templates.

IMHO after the appropriate changes are merged, we can start with the new workflow officially.

In case it is not clear, I'm not volunteering to update those documents, because as I mentioned before I'm not sure it is a good idea.

But my review comment still applies: having all those from_parent indirections results also in missing type annotations in general, and therefore those wrappers should really have them.

Fair enough. 👍

RonnyPfannschmidt · 2020-01-22T21:25:39Z

at first glance it seems like mymy refuses to handle this,

even for from_parent(cls: Type[N] ...) -> N - will take another dig tommorow

RonnyPfannschmidt · 2020-01-23T09:16:54Z

@bluetech ping - do you have any idea how to declare named constructors that correctly take the current type and return it, as far as i can tell my attempt fails

blueyed · 2020-01-24T17:04:16Z

src/_pytest/python.py

@@ -45,6 +47,9 @@
 from _pytest.warning_types import PytestCollectionWarning
 from _pytest.warning_types import PytestUnhandledCoroutineWarning

+if False:  # TYPE_CHECKING
+    from typing import Type


Please use from _pytest.compat import TYPE_CHECKING.

RonnyPfannschmidt · 2020-01-24T21:09:06Z

calling off the typing after repeated issues with mypy on classlevel super and passing over/around named constructor input/output types

nicoddemus · 2020-01-24T21:26:42Z

@blueyed

I've started some WIP on reverting it myself in blueyed#143, but have not looked into it a while.

Do still have plans to do so? I'm asking to know if we should hold on to release from_parent to the world if you think you can get rid of it and appease @RonnyPfannschmidt's concerns this refactoring; even a proof of concept that shows that it is possible and describes your strategy would be fine for us to decide on that I think. 👍

bluetech · 2020-01-25T13:09:01Z

@bluetech ping - do you have any idea how to declare named constructors that correctly take the current type and return it, as far as i can tell my attempt fails

I looked at it. Overall the metaclass constructions seems quite problematic for typing, from two perspectives:

It seems not possible to type __call__ and _create, as you found.
The *k, **kw arguments lose type safety.

So I now regret having proposed it in the first place!

What do you think about trying the other approach I suggested in the comment #5975 (comment)? Namely, to add an internal underscored keyword-only argument to the Node __init__()s, which only pytest itself is supposed to set, and if it is not set, show the deprecation warning, and eventually remove. This shouldn't have typing issues.

One thing the metaclass approach does is that it applies recursively to all Node subclasses, so that it forces all subclasses to not have a public constructor either, but use a classmethod instead. Is this an explicit goal? I would hope that just the deprecation warning will be enough to guide plugin authors to the desirable construction patterns (requiring parent etc.).

RonnyPfannschmidt · 2020-01-25T15:31:31Z

@bluetech one of the intents is to take away public constructors,
the alternative is a chain of a dozen breaking releases that sort out ctor logic and will in the end still require named ctors to fill in data correctly

bluetech · 2020-01-25T15:47:36Z

@RonnyPfannschmidt I am thinking about something like this:

class Node:  # No metaclass
    def __init__(
        self,
        name,
        parent: Optional["Node"] = None,
        config: Optional[Config] = None,
        session: Optional["Session"] = None,
        fspath: Optional[py.path.local] = None,
        nodeid: Optional[str] = None,
        *,
        _internal_to_pytest_i_know_what_i_am_doing=False,
    ) -> None:
        if not _internal_to_pytest_i_know_what_i_am_doing:
            # Raise deprecation warning, can error in the future.
        ....

    @classmethod
    def from_parent(cls, parent, *, name):
        return cls(parent=parent, name=name, _internal_to_pytest_i_know_what_i_am_doing=True)

So if the ctor is used directly, without setting the internal kwarg, can do whatever is needed. And hopefully the kwarg has a scary enough name that people don't try to work around it :)

RonnyPfannschmidt · 2020-01-25T15:55:35Z

@bluetech that is a problem for subclasssing however

bluetech · 2020-01-25T16:07:23Z

@RonnyPfannschmidt why is that? (I might have a guess but I'm not sure). And do you mean internal (pytest repo) subclasses, or external?

RonnyPfannschmidt · 2020-01-25T18:02:14Z

@bluetech both, until a subclass initialization protocol is established (atm the ctors do a lot of magic

bluetech · 2020-01-25T20:49:28Z

One thing I'm missing is who is the intended users of this API (I don't have much experience with the pytest ecosystem). Without it it's hard to tell what's the best way to "protect" it.

(A) Regular pytest users.
(B) Plugins which only use pre-constructed nodes.
(C) Plugins which invent new node types, like doctest and unittest plugins but external.
(D) Plugins which construct existing node types (e.g. python.Function()).

(A) and (B) I assume are irrelevant.

(C) cases already need to call super().__init__() in their own __init__() so it's not really possible to abstract away the details. So it's not really relevant for them, except maybe encouraging them to add factory methods of their own.

This leaves (D). But are there actually plugins which do that (construct existing node types)?

RonnyPfannschmidt · 2020-01-26T09:26:47Z

C and D are targeted,
a subclass initialization protocol is needed

the details are outlined on the ml

bluetech · 2020-01-26T17:51:08Z

But (C) still needs to call super().__init__(), so how does the abstraction work in that case? For example, in this PR, DoctestItem.__init__() still calls super().__init__(), so it needs to know what its superclass Item's __init__() looks like, so you can't change Item.__init__() since it's API (changing would break (C)).

If we remove (C) from consideration, then the problem for subclassing is not relevant, and the internal kwarg approach becomes viable.

RonnyPfannschmidt · 2020-01-26T20:00:05Z

@bluetech thats why a subclass initialization protocol is necessary, you cant hide away init, but you can affect a great deal of interaction with it - and still, __init__ as we know it has to go, as we have quite unreasonable logic in the base hierarchy

RonnyPfannschmidt · 2020-01-26T21:02:47Z

anyway, this pr is about disallowing certain arguments, not reiterating the details, that should be done on the ml and with a alternative that grasps the issues with the different ctors and how they entangle

RonnyPfannschmidt · 2020-01-28T13:55:06Z

Ps, I don't consider the separate branch sustainable for me

nicoddemus · 2020-01-29T22:20:56Z

Guys decided to merge this PR because features already contains related changes, this PR only adds to them. If we decide to revert this/replace it somehow, it will still need to be done for other commits besides the ones here.

blueyed · 2020-02-07T21:15:32Z

@bluetech maybe this is relevant? davidhalter/jedi-vim#995 (comment)

RonnyPfannschmidt changed the base branch from master to features December 31, 2019 20:25

RonnyPfannschmidt force-pushed the fix-6341-disallow-session-config-in-fromparent branch from fdcd45f to 4fcacde Compare December 31, 2019 20:42

bluetech reviewed Jan 1, 2020

View reviewed changes

RonnyPfannschmidt mentioned this pull request Jan 1, 2020

introduce FunctionDefinition with a scope to the official collection tree #3926

Open

nicoddemus reviewed Jan 3, 2020

View reviewed changes

nicoddemus requested changes Jan 4, 2020

View reviewed changes

nicoddemus approved these changes Jan 8, 2020

View reviewed changes

RonnyPfannschmidt changed the title ~~Fix 6341 disallow session config in fromparent~~ [WIP] Fix 6341 disallow session config in fromparent Jan 9, 2020

blueyed reviewed Jan 9, 2020

View reviewed changes

changelog/6387.feature.rst Outdated Show resolved Hide resolved

RonnyPfannschmidt force-pushed the fix-6341-disallow-session-config-in-fromparent branch from 86a581f to 2958129 Compare January 15, 2020 11:52

RonnyPfannschmidt changed the title ~~[WIP] Fix 6341 disallow session config in fromparent~~ Fix 6341 disallow session config in fromparent Jan 15, 2020

RonnyPfannschmidt force-pushed the fix-6341-disallow-session-config-in-fromparent branch from 2958129 to a852ba7 Compare January 15, 2020 11:55

fix pytest-dev#6341 - disallow session/config in Node.from_parent

8ba0b7b

RonnyPfannschmidt force-pushed the fix-6341-disallow-session-config-in-fromparent branch from a852ba7 to 8ba0b7b Compare January 15, 2020 12:00

RonnyPfannschmidt commented Jan 15, 2020

View reviewed changes

blueyed reviewed Jan 21, 2020

View reviewed changes

blueyed suggested changes Jan 21, 2020

View reviewed changes

blueyed mentioned this pull request Jan 23, 2020

Refactor Session._initialparts to have a more explicit type #6547

Merged

RonnyPfannschmidt force-pushed the fix-6341-disallow-session-config-in-fromparent branch from 11e48b3 to 8cfc5a1 Compare January 24, 2020 13:00

blueyed reviewed Jan 24, 2020

View reviewed changes

RonnyPfannschmidt force-pushed the fix-6341-disallow-session-config-in-fromparent branch from 8cfc5a1 to 8ba0b7b Compare January 24, 2020 21:07

blueyed mentioned this pull request Jan 25, 2020

RFC: Streamlining pytest's git workflow #6571

Merged

bluetech mentioned this pull request Jan 27, 2020

Type annotate main.py and some parts related to collection #6556

Closed

nicoddemus merged commit 64ab68f into pytest-dev:features Jan 29, 2020

blueyed mentioned this pull request Jan 31, 2020

Remove Node.from_parent/.from_config indirection #6645

Closed

3 tasks

RonnyPfannschmidt deleted the fix-6341-disallow-session-config-in-fromparent branch June 6, 2021 09:31

	As part of that session/config are already disallowed parameters and as we work on the details we might need disallow a few more as well.
	As part of that session/config are already disallowed parameters and as we work on the details we might need to disallow a few more as well.

Fix 6341 disallow session config in fromparent #6387

Fix 6341 disallow session config in fromparent #6387

Conversation

RonnyPfannschmidt commented Dec 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Jan 9, 2020

nicoddemus commented Jan 9, 2020

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

nicoddemus commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueyed left a comment • edited Loading

Choose a reason for hiding this comment

blueyed commented Jan 21, 2020

nicoddemus commented Jan 21, 2020

RonnyPfannschmidt commented Jan 21, 2020

nicoddemus commented Jan 21, 2020

RonnyPfannschmidt commented Jan 21, 2020

nicoddemus commented Jan 21, 2020

blueyed commented Jan 22, 2020

blueyed commented Jan 22, 2020

nicoddemus commented Jan 22, 2020

RonnyPfannschmidt commented Jan 22, 2020

RonnyPfannschmidt commented Jan 23, 2020

Choose a reason for hiding this comment

RonnyPfannschmidt commented Jan 24, 2020

nicoddemus commented Jan 24, 2020

bluetech commented Jan 25, 2020

RonnyPfannschmidt commented Jan 25, 2020

bluetech commented Jan 25, 2020

RonnyPfannschmidt commented Jan 25, 2020

bluetech commented Jan 25, 2020

RonnyPfannschmidt commented Jan 25, 2020

bluetech commented Jan 25, 2020

RonnyPfannschmidt commented Jan 26, 2020

bluetech commented Jan 26, 2020

RonnyPfannschmidt commented Jan 26, 2020

RonnyPfannschmidt commented Jan 26, 2020

RonnyPfannschmidt commented Jan 28, 2020

nicoddemus commented Jan 29, 2020

blueyed commented Feb 7, 2020

RonnyPfannschmidt commented Dec 31, 2019 •

edited

Loading

RonnyPfannschmidt Jan 2, 2020 •

edited

Loading

blueyed left a comment •

edited

Loading