Drop support for Python 3.5 #184

drasmuss · 2020-10-21T18:35:08Z

And switch to some of the nice 3.6+ features (pathlib, ordered dicts, and f-strings).

hunse · 2020-11-24T14:01:38Z

nengo_dl/__init__.py

@@ -20,7 +20,7 @@

 There are two options for getting NengoDL working:

- Upgrade to Python >= 3.5
+- Upgrade to Python >= 3.6

 - Install an older version of NengoDL:


I'll change this "older version of NengoDL" to 3.4.0, since that's the latest to support Python 3.5 (if they're using earlier than 3.5, they'll have to figure things out themselves).

hunse · 2020-11-24T14:14:09Z

docs/examples/keras-to-snn.ipynb

@@ -282,7 +282,7 @@
    "            plt.ylabel(\"Firing rates (Hz)\")\n",
    "        plt.xlabel(\"Timestep\")\n",
    "        plt.title(\n",
-    "            \"Neural activities (conv0 mean=%dHz max=%dHz)\" % (rates.mean(), rates.max())\n",
+    "            f\"Neural activities (conv0 mean={rates.mean()}Hz max={rates.max()}Hz)\"\n",


Should have spaces before units (Hz). Also, we were formatting these as integers before; I might recommend one decimal place.

hunse · 2020-11-24T14:38:59Z

nengo_dl/tests/test_simulator.py

@@ -835,7 +838,9 @@ def test_profile(Simulator, mode, tmp_path):
    with Simulator(net) as sim:
        # note: TensorFlow bug if using profile_batch=1, see
        # https://github.com/tensorflow/tensorflow/issues/37543
-        callback = callbacks.TensorBoard(log_dir=tmp_path / "profile", profile_batch=2)
+        callback = callbacks.TensorBoard(
+            log_dir=str(tmp_path / "profile"), profile_batch=2


Why do we need an explicit str call here, but not earlier in this file when we create a tf.keras.callbacks.TensorBoard? (callbacks.TensorBoard uses the same constructor, so I'd think they'd be the same.)

Hmm I'm not sure. Generally I started by just passing in a Path wherever I could, and only converted it to a string if that failed. But I don't know why this one would have failed and the other didn't, maybe I just missed this one.

Tried removing the str and it fails. No idea why it fails here and not in the other place, but either way it's all on the TensorFlow side of things so all we can do is cast the paths to strings where required.

drasmuss · 2020-11-26T20:37:15Z

nengo_dl/neuron_builders.py

@@ -5,7 +5,6 @@
 import contextlib
 import logging
 import warnings
-from collections import OrderedDict


I'm curious why pylint didn't detect these unused imports 🤷

hunse · 2020-12-01T17:15:36Z

I've pushed the same fix here that I did in the Nengo PR, which is that we can't use f-strings in the code in __init__.py that errors if we're on Python 3.5 or less, because that code is meant to be executed by earlier python versions. Since version.py is imported before that, we also can't use them in version.py.

If this looks good to you @drasmuss, I'll squash and merge.

drasmuss · 2020-12-01T18:16:59Z

We could probably just drop that whole version warning as well. pip has supported python_requires (which is a better way of doing this) since version 9.0, which has been out since 2016. We added this code back in 2018, when it was plausible that people might still be using a pip version that old. But it seems pretty unlikely that people are installing modern versions of nengo-dl with super old pip versions at this point. And pip already gives a warning telling you that you should upgrade your pip version whenever you try to install a package using one of those old versions.

hunse · 2020-12-01T18:47:05Z

Sure, sounds good. We still need to not use f-strings in version.py, though, because we import that in setup.py. And we should remove that f-string from setup.py too. (Right now if you try to pip install -e . in Python 3.5, you get a lengthy syntax error, rather than the concise error about needing a newer Python version.)

drasmuss · 2020-12-01T18:55:40Z

Maybe add a comment in setup/version.py explaining why we aren't using f-strings as well (I can see someone like me forgetting why we did this in the future).

Alternatively, I wonder if there's a way to monkeypatch out the python 3.6 features and try to parse setup.py, to catch errors like this. That seems like it'd be hard to do, but perhaps Python has some kind of built in support for evaling files with a given python version?

tbekolay · 2020-12-01T18:57:09Z

Another thing we could do is clean up nengo/nengo-bones#39 which puts the version in .nengobones.yml, which means we don't need to import version.py at all.

drasmuss · 2020-12-01T18:58:50Z

Looks like we might be able to use ast.parse("setup.py", feature_version=(3, 5)) to test setup.py for compatibility https://docs.python.org/3/library/ast.html#ast.parse

hunse · 2020-12-01T19:03:36Z

Where would we put that ast.parse test? IMO, it's easier just to keep setup.py working with earlier Pythons. (Adding the comment is definitely a good idea.)

I do like the idea of moving the version into .nengobones.yml. I'm happy to dust off that PR now, if we think it's high enough priority, or we can leave it as future work.

drasmuss · 2020-12-01T19:11:40Z

Where would we put that ast.parse test? IMO, it's easier just to keep setup.py working with earlier Pythons. (Adding the comment is definitely a good idea.)

Probably tests/test_setup.py. And this would be ensuring that we're keeping setup.py working with earlier Pythons, it's not opposed to that I don't think.

hunse · 2020-12-01T19:24:40Z

Ahh, got it, I misunderstood. I thought you meant we would use f-strings in setup.py, but then have some check that would run before setup.py on a pip install, to tell the user that their python is too old. Sure, I'll look at adding a test.

drasmuss · 2020-12-02T15:00:14Z

Pushed a minor update to the test, fixups LGTM!

nengo_dl/tests/test_setup.py

hunse · 2020-12-02T16:12:52Z

I added a fix for some warnings that we weren't importing Mapping from collections.abc. Once the tests pass, I'll merge.

5f92079 - Fix codecov missing files with same names 01104fd - Extra f-string replacements 2f83812 - Drop support for Python 3.5

- Switch to using pathlib - Stop using OrderedDict - Switch to using f-strings

hunse reviewed Nov 24, 2020

View reviewed changes

hunse force-pushed the py35 branch from 470066f to 76e81df Compare November 24, 2020 15:03

drasmuss commented Nov 26, 2020

View reviewed changes

drasmuss force-pushed the py35 branch from 76e81df to f0314ab Compare November 27, 2020 16:55

hunse force-pushed the py35 branch from ba9f721 to cf34c3c Compare November 30, 2020 15:26

drasmuss force-pushed the py35 branch from 84b6084 to c010a98 Compare December 2, 2020 14:56

hunse reviewed Dec 2, 2020

View reviewed changes

nengo_dl/tests/test_setup.py Outdated Show resolved Hide resolved

hunse force-pushed the py35 branch from f2b0b04 to 56ca221 Compare December 2, 2020 16:13

NengoBones and others added 4 commits December 2, 2020 16:52

Automatic update to NengoBones 5f92079

d0d3155

5f92079 - Fix codecov missing files with same names 01104fd - Extra f-string replacements 2f83812 - Drop support for Python 3.5

Add learning_phase config documentation

cf9bb1f

Drop support for Python 3.5

1540c60

- Switch to using pathlib - Stop using OrderedDict - Switch to using f-strings

Get Mapping from collections.abc

a1a0f36

hunse force-pushed the py35 branch from 56ca221 to a1a0f36 Compare December 2, 2020 17:30

hunse merged commit a1a0f36 into master Dec 2, 2020

hunse deleted the py35 branch December 2, 2020 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop support for Python 3.5 #184

Drop support for Python 3.5 #184

drasmuss commented Oct 21, 2020

hunse Nov 24, 2020

hunse Nov 24, 2020

hunse Nov 24, 2020

drasmuss Nov 26, 2020

drasmuss Nov 27, 2020

drasmuss Nov 26, 2020

hunse commented Dec 1, 2020

drasmuss commented Dec 1, 2020 •

edited

Loading

hunse commented Dec 1, 2020

drasmuss commented Dec 1, 2020

tbekolay commented Dec 1, 2020

drasmuss commented Dec 1, 2020 •

edited

Loading

hunse commented Dec 1, 2020 •

edited

Loading

drasmuss commented Dec 1, 2020

hunse commented Dec 1, 2020

drasmuss commented Dec 2, 2020

hunse commented Dec 2, 2020

Drop support for Python 3.5 #184

Drop support for Python 3.5 #184

Conversation

drasmuss commented Oct 21, 2020

hunse Nov 24, 2020

Choose a reason for hiding this comment

hunse Nov 24, 2020

Choose a reason for hiding this comment

hunse Nov 24, 2020

Choose a reason for hiding this comment

drasmuss Nov 26, 2020

Choose a reason for hiding this comment

drasmuss Nov 27, 2020

Choose a reason for hiding this comment

drasmuss Nov 26, 2020

Choose a reason for hiding this comment

hunse commented Dec 1, 2020

drasmuss commented Dec 1, 2020 • edited Loading

hunse commented Dec 1, 2020

drasmuss commented Dec 1, 2020

tbekolay commented Dec 1, 2020

drasmuss commented Dec 1, 2020 • edited Loading

hunse commented Dec 1, 2020 • edited Loading

drasmuss commented Dec 1, 2020

hunse commented Dec 1, 2020

drasmuss commented Dec 2, 2020

hunse commented Dec 2, 2020

drasmuss commented Dec 1, 2020 •

edited

Loading

drasmuss commented Dec 1, 2020 •

edited

Loading

hunse commented Dec 1, 2020 •

edited

Loading