Feat: Default to no multiprocessing. #319

giulioungaretti · 2016-08-29T16:49:31Z

Fixes #314.

Changes proposed in this pull request:

defaults to no multiprocessing
raises warnings if multiprocessing is used
fixes some test

This is a boring PR, but f.ex before merge one needs #315 .

qcodes-bot · 2016-08-29T18:17:17Z

Current coverage is 82.78% (diff: 100%)

Merging #319 into master will decrease coverage by 0.03%

@@             master       #319   diff @@
==========================================
  Files            34         34          
  Lines          3615       3637    +22   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2994       3011    +17   
- Misses          621        626     +5   
  Partials          0          0

Powered by Codecov. Last update da80aa0...011ae1d

giulioungaretti · 2016-08-30T13:14:54Z

And merging soon, unless @alexcjohnson disagrees.

alexcjohnson · 2016-08-30T13:30:52Z

qcodes/instrument/base.py

 import logging
+import time
+import warnings


unused addition?

giulioungaretti · 2016-09-13T12:31:49Z

@alexcjohnson maybe I missed some other things, but I'd say this is done.

alexcjohnson · 2016-09-14T02:34:01Z

.travis.yml

-    - python  qcodes/test.py --skip-coverage
-    - python  qcodes/test.py --mp-spawn
+    - python  qcodes/test.py --skip-coverage -f
+    - python  qcodes/test.py --mp-spawn -f


do we really want to fail fast on travis? I'd think a full failure report is more useful.

Also, if we're not supporting multiprocessing for now anyway, is it really worthwhile running the tests in both modes?

yes, because there are still people that just use multiprocessing, and knowing when we break things is useful.
The -f was just so that people read the output, it seems like it's always ignored, so maybe if it's shorted it's easier ?

Oh right, I thought I was going to see that this PR disabled the multiprocessing tests but it didn't :) Good, so we leave both modes in.

If tests pass (which should be a requirement from now on, yes?), this doesn't shorten it at all. All it really does is shorten a horrendous multi-test failure into a single failure - which is enough to disqualify the commit, and it's nice that you get the feedback sooner, but I would still think the debugging cycle would be quicker if you could see all the failures at once. Of course it would be silly to debug only this way - after you see the error(s) on Travis then you test locally while fixing... dunno, if you prefer this way then leave it, in fact you could even go farther and chain the two commands so if the first fails, the spawn version doesn't even run.

@alexcjohnson it shortens the output of the failure, in general people should test locally but I bet most don't .
We could remove the multiprocessing tests all together :D But that's for another PR.

giulioungaretti · 2016-09-14T07:37:59Z

@alexcjohnson on a second though I think I abused the warnings a bit :D I think I will just keep it in the qcodes process !

giulioungaretti · 2016-09-30T21:33:29Z

Oh wow, merging this is going to be a bit tricky.

giulioungaretti · 2016-10-01T21:05:28Z

@alexcjohnson I think this is ready to merge.

alexcjohnson · 2016-10-02T02:59:37Z

qcodes/data/data_set.py

+        data_manager (Optional[bool]): use a manager for the
+            ``DataServer`` that offloads storage and syncing of this Defaults
+            to  ``False`` i.e. this ``DataSet`` will store itself without extra
+            processes.  ``DataSet``. Set to ``True`` to use the default from


What is the word DataSet doing in this line?

alexcjohnson · 2016-10-02T02:59:44Z

qcodes/data/data_set.py

+        data_manager (Optional[bool]): use a manager for the
+            ``DataServer`` that offloads storage and syncing of this Defaults
+            to  ``False`` i.e. this ``DataSet`` will store itself without extra
+            processes.  ``DataSet``. Set to ``True`` to use the default from


alexcjohnson · 2016-10-02T03:01:43Z

qcodes/tests/test_instrument.py

-        cls.gates = MockGates(model=cls.model)
-        cls.source = MockSource(model=cls.model)
-        cls.meter = MockMeter(model=cls.model, keep_history=False)
+        import pdb


debugging leftover?

alexcjohnson · 2016-10-02T03:01:53Z

qcodes/tests/test_instrument.py


    def setUp(self):
+        import pdb


alexcjohnson

Just a couple of little cleanup comments, but after those I am happy with it.

Because multiprocessing is not open for public use, unless one really wants to use it. The test are incredibly coupled, so they needed some massaging too. Closes #314.

qcodes-bot · 2016-10-02T20:26:21Z

Here is an overview of what got changed by this pull request:

Clones added
============
- qcodes/tests/test_loop.py  6

See the complete overview on Codacy

Merge: a476f61 3ef83bc Author: Giulio Ungaretti <giulio.ungaretti@gmail.com> Merge pull request #319 from qdev-dk/feature/remove_mp

alexcjohnson · 2016-10-02T23:26:28Z

qcodes/tests/test_instrument.py

@@ -31,6 +31,7 @@ def __init__(self, *args, **kwargs):
                           vals=Numbers(-10, 10), step=0.2,
                           delay=0.01,
                           max_delay='forever')
+        self.crahs()


@giulioungaretti I missed this earlier - what's it about? We shouldn't ever get here, as add_parameter should error first...

yeah it's some leftovers, this pr took too long :D

4696: Update pytest-mock requirement from ~=3.9.0 to ~=3.10.0 r=jenshnielsen a=dependabot[bot] Updates the requirements on [pytest-mock](https://github.com/pytest-dev/pytest-mock) to permit the latest version. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-mock/releases">pytest-mock's releases</a>.</em></p> <blockquote> <h2>v3.10.0</h2> <ul> <li>Added new <code>mocker.stop(m)</code> method to stop specific <code>mocker.patch</code> or <code>mocker.spy</code> calls (<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/pull/319">#319</a>).</li> </ul> </blockquote> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-mock/blob/main/CHANGELOG.rst">pytest-mock's changelog</a>.</em></p> <blockquote> <h2>3.10.0 (2022-10-05)</h2> <ul> <li>Added new <code>mocker.stop(m)</code> method to stop specific <code>mocker.patch</code> or <code>mocker.spy</code> calls (<code>[#319](https://github.com/pytest-dev/pytest-mock/issues/319)</code>_).</li> </ul> <p>.. _<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/319">#319</a>: <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/pull/319">pytest-dev/pytest-mock#319</a></p> <h2>3.9.0 (2022-09-28)</h2> <ul> <li>Expose <code>NonCallableMagicMock</code> via the <code>mocker</code> fixture (<code>[#318](https://github.com/pytest-dev/pytest-mock/issues/318)</code>_).</li> </ul> <p>.. _<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/318">#318</a>: <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/pull/318">pytest-dev/pytest-mock#318</a></p> <h2>3.8.2 (2022-07-05)</h2> <ul> <li>Fixed <code>AsyncMock</code> support for Python 3.7+ in <code>mocker.async_stub</code> (<code>[#302](https://github.com/pytest-dev/pytest-mock/issues/302)</code>_).</li> </ul> <p>.. _<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/302">#302</a>: <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/pull/302">pytest-dev/pytest-mock#302</a></p> <h2>3.8.1 (2022-06-24)</h2> <ul> <li>Fixed regression caused by an explicit <code>mock</code> dependency in the code (<code>[#298](https://github.com/pytest-dev/pytest-mock/issues/298)</code>_).</li> </ul> <p>.. _<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/298">#298</a>: <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/298">pytest-dev/pytest-mock#298</a></p> <h2>3.8.0 (2022-06-24)</h2> <ul> <li>Add <code>MockerFixture.async_mock</code> method. Thanks <code>`@PerchunPak</code>_` for the PR (<code>[#296](https://github.com/pytest-dev/pytest-mock/issues/296)</code>_).</li> </ul> <p>.. _<a href="https://github.com/PerchunPak"><code>`@PerchunPak</code></a>:` <a href="https://github.com/PerchunPak">https://github.com/PerchunPak</a> .. _<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/296">#296</a>: <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/pull/296">pytest-dev/pytest-mock#296</a></p> <h2>3.7.0 (2022-01-28)</h2> <ul> <li>Python 3.10 now officially supported.</li> <li>Dropped support for Python 3.6.</li> </ul> <h2>3.6.1 (2021-05-06)</h2> <ul> <li>Fix <code>mocker.resetall()</code> when using <code>mocker.spy()</code> (<code>[#237](https://github.com/pytest-dev/pytest-mock/issues/237)</code><em>). Thanks <code>`@blaxter</code></em>` for the report and <code>`@shadycuz</code>_` for the PR.</li> </ul> <p>.. _<a href="https://github.com/blaxter"><code>`@blaxter</code></a>:` <a href="https://github.com/blaxter">https://github.com/blaxter</a> .. _<a href="https://github.com/shadycuz"><code>`@shadycuz</code></a>:` <a href="https://github.com/shadycuz">https://github.com/shadycuz</a> .. _<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/237">#237</a>: <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/237">pytest-dev/pytest-mock#237</a></p>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pytest-dev/pytest-mock/commit/6c03dfd4240de4a178bab67c0a32fba28d8bcf91"><code>6c03dfd</code></a> Release 3.10.0</li> <li><a href="https://github.com/pytest-dev/pytest-mock/commit/fbb5039d7269c34705a7dab39f2a2cea92111859"><code>fbb5039</code></a> Implement selective un-spying and un-patching (<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/319">#319</a>)</li> <li><a href="https://github.com/pytest-dev/pytest-mock/commit/a1c7421daad8d6bc433db28a5df1b01bd0a93222"><code>a1c7421</code></a> [pre-commit.ci] pre-commit autoupdate (<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/321">#321</a>)</li> <li><a href="https://github.com/pytest-dev/pytest-mock/commit/4f2703c94ce4838befc1ffef1c3bdca4ac1eccf2"><code>4f2703c</code></a> Add Python 3.11 support (<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/320">#320</a>)</li> <li><a href="https://github.com/pytest-dev/pytest-mock/commit/1e2001fb78aeb42ae068cc0c17075016d47fa1a7"><code>1e2001f</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/318">#318</a> from willfrey/patch-2</li> <li><a href="https://github.com/pytest-dev/pytest-mock/commit/798f07e99773b05d284be6f4cb32e79d532965b5"><code>798f07e</code></a> [pre-commit.ci] pre-commit autoupdate (<a href="https://github-redirect.dependabot.com/pytest-dev/pytest-mock/issues/317">#317</a>)</li> <li>See full diff in <a href="https://github.com/pytest-dev/pytest-mock/compare/v3.9.0...v3.10.0">compare view</a></li> </ul> </details> <br /> You can trigger a rebase of this PR by commenting ``@dependabot` rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - ``@dependabot` rebase` will rebase this PR - ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it - ``@dependabot` merge` will merge this PR after your CI passes on it - ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it - ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging - ``@dependabot` reopen` will reopen this PR if it is closed - ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

alexcjohnson reviewed Aug 30, 2016
View reviewed changes

qcodes/instrument/base.py

import logging

import time

import warnings

Copy link

Contributor

alexcjohnson Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused addition?

giulioungaretti force-pushed the feature/remove_mp branch from edad6aa to 099dd49 Compare September 13, 2016 12:10

giulioungaretti added p1 mulitprocessing labels Sep 13, 2016

giulioungaretti added this to the v0.1 colossal cleanup milestone Sep 13, 2016

giulioungaretti force-pushed the feature/remove_mp branch from 099dd49 to de3829a Compare September 13, 2016 12:31

alexcjohnson reviewed Sep 14, 2016
View reviewed changes

alexcjohnson mentioned this pull request Sep 15, 2016

Ag liveplot #315

Merged

giulioungaretti force-pushed the feature/remove_mp branch 2 times, most recently from b23aca5 to 1af588c Compare October 1, 2016 20:41

alexcjohnson reviewed Oct 2, 2016

View reviewed changes

qcodes/tests/test_instrument.py

def setUp(self):

import pdb

Copy link

Contributor

alexcjohnson Oct 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

alexcjohnson approved these changes Oct 2, 2016

View reviewed changes

giulioungaretti force-pushed the feature/remove_mp branch from 1af588c to 4b8d3f3 Compare October 2, 2016 19:39

giulioungaretti force-pushed the feature/remove_mp branch from 4b8d3f3 to 6750c31 Compare October 2, 2016 19:40

Feat: Default to no multiprocessing.

3ef83bc

Because multiprocessing is not open for public use, unless one really wants to use it. The test are incredibly coupled, so they needed some massaging too. Closes #314.

giulioungaretti force-pushed the feature/remove_mp branch from 6750c31 to 3ef83bc Compare October 2, 2016 20:24

giulioungaretti merged commit 2068558 into master Oct 2, 2016

giulioungaretti deleted the feature/remove_mp branch October 2, 2016 20:27

giulioungaretti pushed a commit that referenced this pull request Oct 2, 2016

Generated gh-pages for commit 2068558

d848bdb

Merge: a476f61 3ef83bc Author: Giulio Ungaretti <giulio.ungaretti@gmail.com> Merge pull request #319 from qdev-dk/feature/remove_mp

alexcjohnson reviewed Oct 2, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Default to no multiprocessing. #319

Feat: Default to no multiprocessing. #319

giulioungaretti commented Aug 29, 2016 •

edited

qcodes-bot commented Aug 29, 2016

giulioungaretti commented Aug 30, 2016

alexcjohnson Aug 30, 2016

giulioungaretti commented Sep 13, 2016

alexcjohnson Sep 14, 2016

giulioungaretti Sep 14, 2016

alexcjohnson Sep 15, 2016

giulioungaretti Oct 1, 2016

giulioungaretti commented Sep 14, 2016

giulioungaretti commented Sep 30, 2016

giulioungaretti commented Oct 1, 2016

alexcjohnson Oct 2, 2016

alexcjohnson Oct 2, 2016

alexcjohnson Oct 2, 2016

alexcjohnson Oct 2, 2016

alexcjohnson left a comment

qcodes-bot commented Oct 2, 2016

alexcjohnson Oct 2, 2016

giulioungaretti Oct 4, 2016

Feat: Default to no multiprocessing. #319

Feat: Default to no multiprocessing. #319

Conversation

giulioungaretti commented Aug 29, 2016 • edited

qcodes-bot commented Aug 29, 2016

Current coverage is 82.78% (diff: 100%)

giulioungaretti commented Aug 30, 2016

Choose a reason for hiding this comment

giulioungaretti commented Sep 13, 2016

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

giulioungaretti commented Sep 14, 2016

giulioungaretti commented Sep 30, 2016

giulioungaretti commented Oct 1, 2016

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

alexcjohnson left a comment

Choose a reason for hiding this comment

qcodes-bot commented Oct 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giulioungaretti commented Aug 29, 2016 •

edited