#234 use master/worker terminology #268

feuillemorte · 2018-01-16T18:52:07Z

Use master/worker terminologi in arguments

timj · 2018-01-16T18:59:21Z

xdist/dsession.py

@@ -46,7 +46,7 @@ def __init__(self, config):
        self._failed_collection_errors = {}
        self._active_nodes = set()
        self._failed_nodes_count = 0
-        self._max_slave_restart = self.config.getoption('max_slave_restart')
+        self._max_slave_restart = self.config.getoption('max_worker_restart')


It would be better if the code was also changed to use worker rather than slave. Does this code also support the old command line option to stop everything breaking during transition?

No, I just renamed to new method. Do you need support old value? I'll add support

I'm not in charge here, but I think it would be best if a deprecation notice is used for the old command so that it is still supported for a couple of releases before being dropped.

It would be better if the code was also changed to use worker rather than slave

I didn't change another code bacause it must be changed in pytest repo else. Do you think I need rename all code?

I meant, can we stop using "slave" in the xdist code everywhere, and not just in the messages seen by users? Using worker externally and slave internally seems like a weird inconsistency when the aim is to remove slave nomenclature everywhere. I wasn't saying anything about other pytest repos.

PS Thanks for tackling this. I really do appreciate this getting done since I haven't had a chance to have a go myself.

Ok, I'll remove all "slave" from code, but there is one file which uses in pytest. So, I need to create commit in pytest repo else

@feuillemorte you mean the slaveinput attribute of the config object? I think for that case we can have both names, slaveinput and workerinput which point to the same dictionary so we can keep backward compatibility.

@nicoddemus ok, thank you!

nicoddemus

Thanks a ton @feuillemorte for tackling this, we really appreciate it.

Please see my comments. 😁

nicoddemus · 2018-01-16T20:25:29Z

xdist/plugin.py

@@ -26,8 +26,8 @@ def pytest_addoption(parser):
        help="shortcut for '--dist=load --tx=NUM*popen', "
             "you can use 'auto' here for auto detection CPUs number on "
             "host system")
-    group.addoption('--max-slave-restart', action="store", default=None,
-                    help="maximum number of slaves that can be restarted "
+    group.addoption('--max-worker-restart', action="store", default=None,


IIRC you can pass multiple values here to support aliases:

group.addoption('--max-worker-restart', '--max-slave-restart', action="store", ...

I want to add dest value like:

group.addoption('--max-worker-restart', '--max-slave-restart', action="store", default=None, dest="maxworkerrestart", help="maximum number of workers that can be restarted " "when crashed (set to zero to disable this feature)\n" "'--max-slave-restart' option is deprecated and will be removed in next versions")

Seems good, perhaps just changing "in next versions" to "in a future release". 👍

nicoddemus · 2018-01-16T20:28:26Z

xdist/dsession.py

@@ -46,7 +46,7 @@ def __init__(self, config):
        self._failed_collection_errors = {}
        self._active_nodes = set()
        self._failed_nodes_count = 0
-        self._max_slave_restart = self.config.getoption('max_slave_restart')
+        self._max_slave_restart = self.config.getoption('max_worker_restart')


@feuillemorte you mean the slaveinput attribute of the config object? I think for that case we can have both names, slaveinput and workerinput which point to the same dictionary so we can keep backward compatibility.

…te/pytest-xdist into 234-master-worker-terminology

feuillemorte · 2018-01-17T20:09:04Z

Please, test locally. Tests ran successfuly, but.. :)

nicoddemus

Thanks @feuillemorte for the follow up! 😁

Please take a look at my comments when you have the time.

nicoddemus · 2018-01-17T20:13:08Z

testing/test_remote.py

@@ -26,7 +26,7 @@ def __str__(self):
        return "<EventCall %s(**%s)>" % (self.name, self.kwargs)


-class SlaveSetup:
+class workerSetup:


WorkerSetup

nicoddemus · 2018-01-17T20:13:38Z

testing/test_remote.py

-class TestSlaveInteractor:
-    def test_basic_collect_and_runtests(self, slave):
-        slave.testdir.makepyfile("""
+class TestworkerInteractor:


TestWorkerInteractor

nicoddemus · 2018-01-17T20:14:00Z

xdist/looponfail.py



-class SlaveFailSession:
+class workerFailSession:


WorkerFailSession

nicoddemus · 2018-01-17T20:14:11Z

xdist/remote.py

@@ -14,11 +14,11 @@
 import pytest


-class SlaveInteractor:
+class workerInteractor:


WorkerInteractor

nicoddemus · 2018-01-17T20:14:35Z

xdist/workermanage.py

@@ -201,19 +201,19 @@ def make_reltoroot(roots, args):
    return result


-class SlaveController(object):
+class workerController(object):


WorkerController

nicoddemus · 2018-01-17T20:15:18Z

xdist/workermanage.py

    ENDMARK = -1

    def __init__(self, nodemanager, gateway, config, putevent):
        self.nodemanager = nodemanager
        self.putevent = putevent
        self.gateway = gateway
        self.config = config
-        self.slaveinput = {'slaveid': gateway.id,
-                           'slavecount': len(nodemanager.specs)}
+        self.workerinput = {'workerid': gateway.id,


Please add an alias here for backward compatibility:

# deprecated name, backward compatibility only self.slaveinput = workerinput

this dict also need the old names (slaveid and slavecount)

and we likely need to keep those for around 5 years at least

i propose adding as is and later on following up with a dict subclass that gives deprecation warnings in a few years

Ah good catch, I missed those. 👍

@nicoddemus @RonnyPfannschmidt fixed

nicoddemus · 2018-01-17T20:17:09Z

xdist/remote.py

-    config.slaveoutput = {}
-    interactor = SlaveInteractor(config, channel)
+    config.workerinput = workerinput
+    config.workeroutput = {}


Please add aliases here for backward compatibility:

# deprecated name, backward compatibility only config.slaveinput = config.workerinput config.slaveoutput = config.workeroutput

feuillemorte · 2018-01-17T20:19:02Z

how to use linter? :)
tox -e linting ERROR: unknown environment 'linting'

nicoddemus · 2018-01-17T20:30:51Z

how to use linter? :)

tox -l shows all environments, in pytest-xdist the linter is called flakes:

tox -e flakes

nicoddemus · 2018-01-17T20:58:43Z

Thanks @feuillemorte for the quick update!

LGTM so far, let's wait for @RonnyPfannschmidt's review.

feuillemorte · 2018-01-18T09:11:28Z

@timj please, approve too

Related to #234

nicoddemus · 2018-01-30T21:13:50Z

Added an explicit test for backward compatibility of the old slaveinput name.

RonnyPfannschmidt

i believe we covered all that needs covering now
so this is good to go, thanks 👍

i do have a lingering fear that a minor detail is missing, but thise should be sortable very easily

RonnyPfannschmidt · 2018-02-06T09:49:28Z

@feuillemorte great work, thanks again 👍

ionelmc · 2018-02-26T12:44:18Z

Do we really need to also rename the --max-slave-restart? I don't really like the churn of editing config files in a dozen projects.

nicoddemus · 2018-02-26T13:13:10Z

--max-slave-restart was kept as an alias to --max-worker-restart:

pytest-xdist/xdist/plugin.py

Lines 29 to 30 in 63b329c

    
           group.addoption('--max-worker-restart', '--max-slave-restart', action="store", default=None, 
        
                           dest="maxworkerrestart",

feuillemorte added 2 commits January 16, 2018 21:46

use master/worker terminology

f1e6dc4

Add changelog entry

7082562

feuillemorte mentioned this pull request Jan 16, 2018

Stop using master/slave terminology #234

Closed

timj reviewed Jan 16, 2018

View reviewed changes

Improve changelog formatting

9efe149

nicoddemus requested changes Jan 16, 2018

View reviewed changes

feuillemorte added 2 commits January 17, 2018 23:07

#234 Added master/worker terminology

4049127

Merge branch '234-master-worker-terminology' of github.com:feuillemor…

9a5a819

…te/pytest-xdist into 234-master-worker-terminology

nicoddemus requested changes Jan 17, 2018

View reviewed changes

#234 Fix flake 8 and fix comments

b04703b

nicoddemus approved these changes Jan 17, 2018

View reviewed changes

nicoddemus requested a review from RonnyPfannschmidt January 17, 2018 20:58

timj approved these changes Jan 18, 2018

View reviewed changes

Add an explicit test for backward compatibility with "config.slaveinput"

ee2a1b4

Related to #234

RonnyPfannschmidt approved these changes Feb 6, 2018

View reviewed changes

#234 Fix comments

6fb0180

nicoddemus merged commit 23ce450 into pytest-dev:master Feb 6, 2018

feuillemorte deleted the 234-master-worker-terminology branch February 6, 2018 09:28

martinmaillard mentioned this pull request Feb 26, 2018

slaveoutput and slaveinput are now called workeroutput and workerinput in pytest-xdist pytest-dev/pytest-cov#191

Closed

asottile mentioned this pull request Jun 10, 2020

evaluate a new primary branch name to replace master pytest-dev/pytest#7344

Closed

ws1088 mentioned this pull request May 17, 2022

support pytest-xdist workinput (1.22.1) nicoulaj/pytest-csv#20

Merged



		class SlaveFailSession:
		class workerFailSession:

#234 use master/worker terminology #268

#234 use master/worker terminology #268

Conversation

feuillemorte commented Jan 16, 2018

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 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

feuillemorte commented Jan 17, 2018

nicoddemus 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

feuillemorte commented Jan 17, 2018

nicoddemus commented Jan 17, 2018

nicoddemus commented Jan 17, 2018

feuillemorte commented Jan 18, 2018

nicoddemus commented Jan 30, 2018

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt commented Feb 6, 2018

ionelmc commented Feb 26, 2018

nicoddemus commented Feb 26, 2018