Use Config.invocation_params for consistent worker initialization #448

nicoddemus · 2019-07-02T22:40:53Z

EDIT

Decided to keep the old way still working for now.

Fix #6
Fix #445

ORIGINAL POST

While investigating a recent node id bug (#445) I decided to play
with config initialization, around the idea of initializing the config
on the remote exactly how we initialize the config object on pytest.main()

One thing that immediately broke all tests was inprocess pytester runs,
but other than that just a few tests failed which I find encouraging.

While this patch is in no way even remotely ready, I would like to get
opinions if the overall idea of initializing the config object
like this is sound.

src/xdist/workermanage.py

RonnyPfannschmidt · 2019-07-03T05:03:53Z

initializing the config the same way everywhere is structurally a good idea

i believe however that the main config initialization process needs some solid and api breaking work to support that sanely

nicoddemus · 2019-07-03T10:56:22Z

i believe however that the main config initialization process needs some solid and api breaking work to support that sanely

You mean _prepareconfig is not suitable for this (other than not being public API of course)?

If we can get this working with _prepareconfig, I think it is still worth releasing this as this will fix quite a number of long standing issues. We can think of a good API for config initialization later down the road so other plugins can use the new API, but given that xdist is maintained by the core team, I don't see any harm on using the internal API as an intermediate step.

RonnyPfannschmidt · 2019-07-03T12:21:52Z

im totally fine with having sometihng that works a little bit better, but we will have to break api from he pytestside a few times to get things work nice

nicoddemus · 2019-07-03T14:33:00Z

Cool, sounds like a plan then.

but we will have to break api from he pytestside a few times to get things work nice

We don't really have an API to properly initialize the config objects from the command line arguments today, do we?

I'm thinking, for starters, an API to create the config object from argsand plugins, exactly as given to pytest.main. Something like:

config = Config.from_args_and_plugins(args, plugins)

But we should brainstorm how to initialize both config and session objects so one can selectively run certain stages (say only collection) using public APIs and without having to manually call hooks. I believe some niche use cases would greatly benefit from it, for example IDE launchers.

nicoddemus · 2019-07-05T22:52:42Z

Great news!

With pytest-dev/pytest#5564, all tests in pytest-xdist and pytest (with xdist of course) pass. 🎉

This leads me to believe this is indeed the way to go. We can try to nail this down after pytest 5.1 gets released.

Decided to keep the old way still working for now. Fix pytest-dev#6 Fix pytest-dev#445

nicoddemus · 2019-07-11T00:05:52Z

@RonnyPfannschmidt I think this is now ready for review. 👍

RonnyPfannschmidt

lovely, great job 👍

ionelmc · 2019-10-03T11:26:33Z

src/xdist/workermanage.py

@@ -236,10 +238,14 @@ def shutting_down(self):
    def setup(self):
        self.log("setting up worker session")
        spec = self.gateway.spec
-        args = self.config.args
+        if hasattr(self.config, "invocation_params"):
+            args = [str(x) for x in self.config.invocation_params.args or ()]


Wouldn't this give unwanted arguments to the worker?

I'm investigating why pytest-cov's test suite is broken on 0.30 and invocation_params.args has lots of stuff while config.args is just the test file.

@nicoddemus
Can you comment, please?
(I'm currently looking into getting pytest-cov's CI to pass again so this would help there)

Sorry @ionelmc and @blueyed, I must have missed this.

Why do you say it would give unwanted arguments to the worker? Using invocation_params it will initialize exactly like the master process (well almost), which solved a problem with initialization of the config file.

@nicoddemus I guess I'm just confused about what's wrong with the latest pytest/xdist/cov combination (it's broken in some situations). Any explanation about how the all works would help me debug it.

Not sure myself, but the latest implementation just forwards all the parameters given in the command line to the workers, making them initialize the same way as the master node.

@nicoddemus the way I understand this the problem is that option_dict is empty in the new "invocation_params mode", thus plugins like pytest-cov don't have the necessary options to initialize. Am I missing something?

Thus plugins like pytest-cov don't have the necessary options to initialize.

Unless pytest-cov is trying to get the options from pytest-xdist structures instead of config.getoption, it should just work. The workers should now be initializing the same way as the master node/without xdist, so all the options should be there.

@nicoddemus the problem is that none of the plugin options are there when stuff like --dist=load --tx=popen//chdir=1 is used. When using -n 1 everything is fine.

ionelmc · 2019-12-07T16:42:42Z

src/xdist/workermanage.py

@@ -236,10 +238,14 @@ def shutting_down(self):
    def setup(self):
        self.log("setting up worker session")
        spec = self.gateway.spec
-        args = self.config.args
+        if hasattr(self.config, "invocation_params"):
+            args = [str(x) for x in self.config.invocation_params.args or ()]


@nicoddemus the problem is that none of the plugin options are there when stuff like --dist=load --tx=popen//chdir=1 is used. When using -n 1 everything is fine.

ionelmc · 2019-12-07T16:49:21Z

src/xdist/workermanage.py

+            option_dict = {}
+        else:
+            args = self.config.args
+            option_dict = vars(self.config.option)
        if not spec.popen or spec.chdir:
            args = make_reltoroot(self.nodemanager.roots, args)


@nicoddemus I think now I correctly identified the cause of the problem. In the new code the args stripping matters cause the old option_dict is no more, while previously this stripping had no effect.

Sorry, what "stripping" you mean? Can you please clarify?

@nicoddemus see #491 - make_reltoroot removes anything that ain't a valid path (like the arguments I need).

nicoddemus requested a review from RonnyPfannschmidt July 2, 2019 22:51

RonnyPfannschmidt reviewed Jul 3, 2019

View reviewed changes

src/xdist/workermanage.py Outdated Show resolved Hide resolved

This was referenced Jul 3, 2019

pytest-xdist is not reporting the same nodeid as pytest does #445

Closed

Introduce Config.invocation_params pytest-dev/pytest#5564

Merged

nicoddemus mentioned this pull request Jul 10, 2019

Migrate to 'black' for consistent styling and formatting pytest-dev/pytest-html#218

Merged

nicoddemus force-pushed the config-init-test branch from 8bfa02a to 6bf9672 Compare July 10, 2019 22:08

Use Config.invocation_params for consistent worker initialization

953a3f0

Decided to keep the old way still working for now. Fix pytest-dev#6 Fix pytest-dev#445

nicoddemus force-pushed the config-init-test branch from 6bf9672 to 953a3f0 Compare July 10, 2019 23:00

nicoddemus changed the title ~~WIP play around with config initialization~~ Use Config.invocation_params for consistent worker initialization Jul 10, 2019

nicoddemus requested a review from RonnyPfannschmidt July 11, 2019 00:05

RonnyPfannschmidt approved these changes Jul 11, 2019

View reviewed changes

nicoddemus merged commit 4642f16 into pytest-dev:master Jul 11, 2019

nicoddemus deleted the config-init-test branch August 3, 2019 11:58

ionelmc reviewed Oct 3, 2019

View reviewed changes

ionelmc mentioned this pull request Nov 6, 2019

test_dist_subprocess_not_collocated fails pytest-dev/pytest-cov#358

Open

blueyed mentioned this pull request Nov 22, 2019

Fix CI pytest-dev/pytest-cov#365

Closed

ionelmc reviewed Dec 7, 2019

View reviewed changes

ionelmc mentioned this pull request Dec 7, 2019

Fix regression with custom arguments being dropped in non-local executions #491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Config.invocation_params for consistent worker initialization #448

Use Config.invocation_params for consistent worker initialization #448

nicoddemus commented Jul 2, 2019 •

edited

RonnyPfannschmidt commented Jul 3, 2019

nicoddemus commented Jul 3, 2019 •

edited

RonnyPfannschmidt commented Jul 3, 2019

nicoddemus commented Jul 3, 2019 •

edited

nicoddemus commented Jul 5, 2019

nicoddemus commented Jul 11, 2019

RonnyPfannschmidt left a comment

ionelmc Oct 3, 2019

blueyed Nov 22, 2019

nicoddemus Nov 23, 2019

ionelmc Nov 23, 2019

nicoddemus Nov 25, 2019

ionelmc Dec 7, 2019

nicoddemus Dec 7, 2019

ionelmc Dec 7, 2019

ionelmc Dec 7, 2019

ionelmc Dec 7, 2019

nicoddemus Dec 7, 2019

ionelmc Dec 7, 2019

Use Config.invocation_params for consistent worker initialization #448

Use Config.invocation_params for consistent worker initialization #448

Conversation

nicoddemus commented Jul 2, 2019 • edited

RonnyPfannschmidt commented Jul 3, 2019

nicoddemus commented Jul 3, 2019 • edited

RonnyPfannschmidt commented Jul 3, 2019

nicoddemus commented Jul 3, 2019 • edited

nicoddemus commented Jul 5, 2019

nicoddemus commented Jul 11, 2019

RonnyPfannschmidt left a 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

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

nicoddemus commented Jul 2, 2019 •

edited

nicoddemus commented Jul 3, 2019 •

edited

nicoddemus commented Jul 3, 2019 •

edited