Fix #1186: list-methods of type-annotated class #1188

drewrisinger · 2018-11-03T00:29:08Z

ARTIQ Pull Request

Description of Changes

Change how pc_rpc process annotations. Essentially annotations -> str(annotations) which can be serialized by pyon.

Related Issue

Closes #1186

Type of Changes

	Type
✓	🐛 Bug fix

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

Use correct spelling and grammar.
Update RELEASE_NOTES.md if there are noteworthy changes, especially if there are changes to existing APIs.
Close/update issues.
Check the copyright situation of your changes and sign off your patches (git commit --signoff, see copyright).

Code Changes

Run flake8 to check code style (follow PEP-8 style). flake8 has issues with parsing Migen/gateware code, ignore as necessary.
Test your changes or have someone test them. Mention what was tested and how.
- Added unittest to check this. Manually tested as well with artiq_rpctool SERVER list-methods
Add and check docstrings and comments
Check, test, and update the unittests in /artiq/test/ or gateware simulations in /artiq/gateware/test

Documentation Changes

None

Git Logistics

Split your contribution into logically separate changes (git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.
Write short & meaningful commit messages. Review each commit for messages (git show). Format:

Turn annotations into a string which PYON can encode.

Test pc_rpc can correctly process an annotated function. Also check pyon can encode & decode the annotated function. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>

sbourdeauducq · 2018-11-03T01:14:26Z

artiq/protocols/pc_rpc.py

@@ -487,6 +487,17 @@ def __init__(self, targets, description=None, builtin_terminate=False,
        else:
            self._noparallel = asyncio.Lock()

+    @staticmethod
+    def _document_function(function: typing.Callable) -> typing.Tuple[dict, str]:


Please remove the type annotations.

Can you explain why the type annotations should be removed? Other than not being used elsewhere in ARTIQ (#1181 (comment))?

There is no downside to using type annotations, it makes the code more understandable, and it helps IDEs with autocomplete. Guido van Rossum supports them, and they are making their way into more projects as Python2 support is dropped. Some projects that use them:

Pylint: Annotate pylint code with type annotations pylint-dev/pylint#2079

Black (formatter): https://github.com/ambv/black/blob/master/black.py

Overview of type hints and their benefits; https://stackoverflow.com/questions/32557920/what-are-type-hints-in-python-3-5

They are not used anywhere else in ARTIQ, they are not used by most other Python projects, they are difficult to write, some functions may return different types depending on the context, and annotations are ignored by the interpreter and are (currently at least) not checked automatically.

Discussion continued below in #1188 (comment)

jordens

Thanks. Apart from the one style thing this LGTM

artiq/protocols/pc_rpc.py

m-labs#1188 (comment)

drewrisinger · 2018-11-05T16:49:08Z

@jordens FYI, I found a few other instances of that same return style in that file.

artiq/artiq/protocols/pc_rpc.py

Line 130 in abad916

return (self.__target_names, self.__description)

artiq/artiq/protocols/pc_rpc.py

Line 239 in abad916

return (self.__target_names, self.__description)

jordens · 2018-11-05T16:52:14Z

@drewrisinger touché. Then either style is fine.

drewrisinger · 2018-11-05T17:04:06Z

@drewrisinger touché. Then either style is fine.

Just wanted to let you know since fixing them would be out of scope for this PR

sbourdeauducq · 2018-11-06T00:50:25Z

artiq/test/test_pc_rpc.py

+            return arg1
+
+        argspec, docstring = pc_rpc.Server._document_function(_annotated_function)
+        print(argspec)  # for debug


Prints in tests are generally "for debug", this comment doesn't provide much info.

sbourdeauducq · 2018-12-01T10:47:19Z

@drewrisinger To clarify my position: while i am opposed to using type annotations inside ARTIQ, merging this and having pc_rpc handle them for user code is totally fine.

sbourdeauducq

See comments above.

drewrisinger · 2019-01-04T21:17:15Z

Continuing the discussion on type annotations:

They are not used anywhere else in ARTIQ,

My understanding is that this is due to the nature of ARTIQ. ARTIQ has received contributions from people with varying levels of programming experience, who might be unaware that type annotations exist.

they are not used by most other Python projects,

This is mostly due to them being a new/less-documented feature in Python 3. Their usage is spreading as more projects transition to Python 3. Similar features like generators & async code are not widely used in beginner projects, but have their usage in large & advanced projects. And projects that do not use them, like matplotlib, scipy, numpy have extensive documentation to explain every function, which ARTIQ lacks.

they are difficult to write,

I disagree: no more difficult than a comment once you know how to use them. Proper usage can be learned in about 10-15 mins for 95% of use cases.

some functions may return different types depending on the context,

Not an issue, and easily handled. Returns can be of the form typing.Union[int, str, ...]

and annotations are ignored by the interpreter and are (currently at least) not checked automatically.

This is one of the main benefits. Type annotations can be statically checked to catch obvious mistakes (e.g. mypy), while still being optional.

In summary, I strongly believe that type annotations are best practice and very useful for documentation and code understandability. Especially in Python when it is nearly impossible to tell what data type a function expects, combined with a general lack of function documentation in ARTIQ. As such, all my code does and will continue to use type annotations, because to my cost-benefit analysis their marginal cost comes with a large comprehensibility benefit for users and future maintainability.

sbourdeauducq · 2019-01-05T02:38:23Z

My understanding is that this is due to the nature of ARTIQ. ARTIQ has received contributions from people with varying levels of programming experience, who might be unaware that type annotations exist.

I started writing ARTIQ and wrote large parts of it, and I am obviously aware of type annotations since I've been using them in the compiler. I just don't want them elsewhere.

Similar features like generators & async code are not widely used in beginner projects, but have their usage in large & advanced projects.

Generators add useful features and are not similar to useless type annotations.

And projects that do not use them, like matplotlib, scipy, numpy have extensive documentation to explain every function, which ARTIQ lacks.

Then write or fund such documentation instead of adding those cryptic type annotations.

As such, all my code does and will continue to use type annotations, because to my cost-benefit analysis their marginal cost comes with a large comprehensibility benefit for users and future maintainability.

As such, your code will continue to get rejected.

sbourdeauducq · 2019-01-05T03:17:00Z

This is one of the main benefits. Type annotations can be statically checked to catch obvious mistakes (e.g. mypy), while still being optional.

If those tools were good, I'd be all for using them. This is why we switched the firmware from C to Rust, for example. But with Python they are half-baked hacks that are not really worth the effort.

jbqubit · 2019-01-08T22:23:39Z

@drewrisinger Thanks for keeping the conversation on this going.

@sbourdeauducq said

I started writing ARTIQ and wrote large parts of it, and I am obviously aware of type annotations since I've been using them in the compiler. I just don't want them elsewhere.

Type annotations were supported in Python 3.5 which was released in autumn 2015. By that time @sbourdeauducq already had spent two years on ARTIQ -- v1 was released May 2016. So it wasn't really an option for most of the early ARTIQ infrastructure.

But with Python they are half-baked hacks that are not really worth the effort.

There's a nice medium post discussing additional benefits of type annotations.

Cleaner code/the code is self-documenting -- Comments tend to wear out and rot, while code is alive and must stay fresh. Change the types of the variables without changing the comment specifying the type — nothing happens. Change it without changing the type annotation? Your static analysis tool, whichever it may be, will shout at you.

Automatic code completion is another great IDE feature that works better with type annotations. The same Medium post provides an example illustrating this. This could really make learning and writing in ARTIQ Python much easier.

sbourdeauducq · 2019-01-09T02:28:07Z

Change it without changing the type annotation? Your static analysis tool, whichever it may be, will shout at you.

When they work! Which isn't always the case with the Python tools unlike e.g. Rust.

@sbourdeauducq

Removed comment per @sbourdeauducq. Clarify variable names b/c they didn't make sense when re-reading months later.

@sbourdeauducq

Type annotations removed per @sbourdeauducq, so no longer used.

sbourdeauducq · 2019-01-10T04:12:38Z

@drewrisinger Thanks!

sbourdeauducq · 2019-01-10T06:30:23Z

@drewrisinger Please run tests before submitting PRs. You ticked the box but your unit test fails.
http://buildbot.m-labs.hk/builders/artiq/builds/2808/steps/python_unittest_1/logs/stdio

sbourdeauducq · 2019-01-10T06:35:18Z

@drewrisinger Please submit a fix.

Fixes bug from 5108ed8. Truth value of multi-element numpy array is not defined. Completes m-labs#1186 and fixes/amends m-labs#1188. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>

drewrisinger · 2019-01-10T20:05:06Z

#1239. Had tested originally, forgot why I couldn't add extra element to array.

Fixes bug from 5108ed8. Truth value of multi-element numpy array is not defined. Completes #1186 and fixes/amends #1188. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>

Fixes bug from 5108ed8. Truth value of multi-element numpy array is not defined. Completes m-labs#1186 and fixes/amends m-labs#1188. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>

drewrisinger added 2 commits November 2, 2018 20:24

pc_rpc: fix m-labs#1186. annotations -> str(annotations)

b173f4f

Turn annotations into a string which PYON can encode.

pc_rpc: test m-labs#1186.

7074615

Test pc_rpc can correctly process an annotated function. Also check pyon can encode & decode the annotated function. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>

sbourdeauducq reviewed Nov 3, 2018

View reviewed changes

jordens requested changes Nov 4, 2018

View reviewed changes

artiq/protocols/pc_rpc.py Outdated Show resolved Hide resolved

style fix: remove () in return

32b0013

m-labs#1188 (comment)

drewrisinger mentioned this pull request Nov 5, 2018

[tools]: Add --version flag to cmd line args #1181

Merged

9 tasks

sbourdeauducq reviewed Nov 6, 2018

View reviewed changes

jordens approved these changes Dec 11, 2018

View reviewed changes

sbourdeauducq requested changes Dec 11, 2018

View reviewed changes

drewrisinger added 3 commits January 9, 2019 12:32

pc_rpc: remove type annotations per @sbourdeauducq

01521b5

test_pc_rpc: clarify test and remove comment

5108ed8

Removed comment per @sbourdeauducq. Clarify variable names b/c they didn't make sense when re-reading months later.

pc_rpc: remove unused typing import

c0bf9f7

Type annotations removed per @sbourdeauducq, so no longer used.

drewrisinger mentioned this pull request Jan 9, 2019

PYON: PYON can't serialize type annotations in list-methods #1186

Closed

sbourdeauducq merged commit 721c6f3 into m-labs:master Jan 10, 2019

drewrisinger mentioned this pull request Jan 10, 2019

test_pc_rpc: fix equality bug (#1188) #1239

Merged

jordens mentioned this pull request Dec 10, 2019

Kasli: allow modifying/adding generic peripherals #1403

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1186: list-methods of type-annotated class #1188

Fix #1186: list-methods of type-annotated class #1188

drewrisinger commented Nov 3, 2018

sbourdeauducq Nov 3, 2018

drewrisinger Nov 5, 2018

sbourdeauducq Nov 6, 2018

drewrisinger Jan 4, 2019

jordens left a comment

drewrisinger commented Nov 5, 2018

jordens commented Nov 5, 2018

drewrisinger commented Nov 5, 2018

sbourdeauducq Nov 6, 2018

sbourdeauducq commented Dec 1, 2018

sbourdeauducq left a comment

drewrisinger commented Jan 4, 2019 •

edited

Loading

sbourdeauducq commented Jan 5, 2019

sbourdeauducq commented Jan 5, 2019

jbqubit commented Jan 8, 2019

sbourdeauducq commented Jan 9, 2019

sbourdeauducq commented Jan 10, 2019

sbourdeauducq commented Jan 10, 2019

sbourdeauducq commented Jan 10, 2019

drewrisinger commented Jan 10, 2019

Fix #1186: list-methods of type-annotated class #1188

Fix #1186: list-methods of type-annotated class #1188

Conversation

drewrisinger commented Nov 3, 2018

ARTIQ Pull Request

Description of Changes

Related Issue

Type of Changes

Steps (Choose relevant, delete irrelevant before submitting)

All Pull Requests

Code Changes

Documentation Changes

Git Logistics

sbourdeauducq Nov 3, 2018

Choose a reason for hiding this comment

drewrisinger Nov 5, 2018

Choose a reason for hiding this comment

sbourdeauducq Nov 6, 2018

Choose a reason for hiding this comment

drewrisinger Jan 4, 2019

Choose a reason for hiding this comment

jordens left a comment

Choose a reason for hiding this comment

drewrisinger commented Nov 5, 2018

jordens commented Nov 5, 2018

drewrisinger commented Nov 5, 2018

sbourdeauducq Nov 6, 2018

Choose a reason for hiding this comment

sbourdeauducq commented Dec 1, 2018

sbourdeauducq left a comment

Choose a reason for hiding this comment

drewrisinger commented Jan 4, 2019 • edited Loading

sbourdeauducq commented Jan 5, 2019

sbourdeauducq commented Jan 5, 2019

jbqubit commented Jan 8, 2019

sbourdeauducq commented Jan 9, 2019

sbourdeauducq commented Jan 10, 2019

sbourdeauducq commented Jan 10, 2019

sbourdeauducq commented Jan 10, 2019

drewrisinger commented Jan 10, 2019

drewrisinger commented Jan 4, 2019 •

edited

Loading