M-Labs/nac3-spec
11
0
Fork 0
Code Issues 14 Pull Requests Releases Wiki Activity

class attributes v instance attributes #1

New Issue
Open
opened 2021-01-19 20:11:41 +08:00 by hartytp · 15 comments
hartytp commented 2021-01-19 20:11:41 +08:00
Collaborator
Copy Link

Checking I understand the proposal...one of the standard python gotchas is the difference in behaviour between class attributes and instance attributes.

Looking at this code

class Foo:
    a: int
    b: int
    def __init__(self, a: int, b: int):
        self.a = a
        self.b = b

it seems like the proposed new artiq python scraps the concept of class attributes (which is fine since we're nowhere near the original python data model anyway) and uses the normal class attribute syntax to define type annotations for instance attributes. Is that about correct? Side note: I actually don't know how the current artiq-python treats class attributes.

How does this play out for classes that have both kernel and non-kernel methods?

Checking I understand the proposal...one of the standard python gotchas is the difference in behaviour between class attributes and instance attributes. Looking at this code ``` class Foo: a: int b: int def __init__(self, a: int, b: int): self.a = a self.b = b ``` it seems like the proposed new artiq python scraps the concept of class attributes (which is fine since we're nowhere near the original python data model anyway) and uses the normal class attribute syntax to define type annotations for instance attributes. Is that about correct? Side note: I actually don't know how the current artiq-python treats class attributes. How does this play out for classes that have both kernel and non-kernel methods?
pca006132 commented 2021-01-19 21:25:53 +08:00
Collaborator
Copy Link

Yes, this is used for instance attributes.

How does this play out for classes that have both kernel and non-kernel methods?

I did not thought about this previously, maybe I would have a look tmr to see how this behaves in current artiq-python. (Side note: I'm not an experienced artiq-python user either...)

I personally prefer to only allow kernel functions for class methods (perhaps except constructor), as it does not make much sense to copy all attributes to the host everytime we use RPC, and making the transfer explicit via parameter passing to other RPCs seems not too bad for me.

Yes, this is used for instance attributes. > How does this play out for classes that have both kernel and non-kernel methods? I did not thought about this previously, maybe I would have a look tmr to see how this behaves in current artiq-python. (Side note: I'm not an experienced artiq-python user either...) I personally prefer to only allow kernel functions for class methods (perhaps except constructor), as it does not make much sense to copy all attributes to the host everytime we use RPC, and making the transfer explicit via parameter passing to other RPCs seems not too bad for me.
hartytp commented 2021-01-19 21:42:30 +08:00
Author
Collaborator
Copy Link

I personally prefer to only allow kernel functions for class methods (perhaps except constructor), as it does not make much sense to copy all attributes to the host everytime we use RPC, and making the transfer explicit via parameter passing to other RPCs seems not too bad for me.

Attributes in classes that have both kernel and host methods are a major footgun in artiq-python IME. Improvement here would be welcome so long as it doesn't become overly restrictive. I do think we need to be careful to gather use-cases/code examples from the current artiq-python before making some of these calls however.

> I personally prefer to only allow kernel functions for class methods (perhaps except constructor), as it does not make much sense to copy all attributes to the host everytime we use RPC, and making the transfer explicit via parameter passing to other RPCs seems not too bad for me. Attributes in classes that have both kernel and host methods are a major footgun in artiq-python IME. Improvement here would be welcome so long as it doesn't become overly restrictive. I do think we need to be careful to gather use-cases/code examples from the current artiq-python before making some of these calls however.
pca006132 commented 2021-01-19 22:17:04 +08:00
Collaborator
Copy Link

I personally prefer to only allow kernel functions for class methods (perhaps except constructor), as it does not make much sense to copy all attributes to the host everytime we use RPC, and making the transfer explicit via parameter passing to other RPCs seems not too bad for me.

Attributes in classes that have both kernel and host methods are a major footgun in artiq-python IME. Improvement here would be welcome so long as it doesn't become overly restrictive. I do think we need to be careful to gather use-cases/code examples from the current artiq-python before making some of these calls however.

Yes, but I'm not sure where to find existing use cases apart from the artiq repo.

> > I personally prefer to only allow kernel functions for class methods (perhaps except constructor), as it does not make much sense to copy all attributes to the host everytime we use RPC, and making the transfer explicit via parameter passing to other RPCs seems not too bad for me. > > Attributes in classes that have both kernel and host methods are a major footgun in artiq-python IME. Improvement here would be welcome so long as it doesn't become overly restrictive. I do think we need to be careful to gather use-cases/code examples from the current artiq-python before making some of these calls however. Yes, but I'm not sure where to find existing use cases apart from the artiq repo.
hartytp commented 2021-01-19 23:19:15 +08:00
Author
Collaborator
Copy Link

Yes, but I'm not sure where to find existing use cases apart from the artiq repo.

Just need to make sure we bring the relevant stakeholders into the discussion (which it seems you are doing already...)

> Yes, but I'm not sure where to find existing use cases apart from the artiq repo. Just need to make sure we bring the relevant stakeholders into the discussion (which it seems you are doing already...)
dhslichter commented 2021-02-17 04:03:44 +08:00
Collaborator
Copy Link

One challenge is that it can be nice to write a method as @portable and then be able to run it on either the host or the kernel. This makes it easy to debug code by running it on the host, and then you can just plop the debugged code into a kernel. While this hides potential pitfalls, it is also nice to be able to do code reuse in this way.

Basically, the warning here is that most people who are used to Python classes and attributes will probably get tripped up by these sorts of restrictions. The point is not that we shouldn't have them, but that we need to figure out ways to make this work nicely for users, and not force them to make copy/paste host and kernel versions of methods if we can help it...

One challenge is that it can be nice to write a method as `@portable` and then be able to run it on either the host or the kernel. This makes it easy to debug code by running it on the host, and then you can just plop the debugged code into a kernel. While this hides potential pitfalls, it is also nice to be able to do code reuse in this way. Basically, the warning here is that most people who are used to Python classes and attributes will probably get tripped up by these sorts of restrictions. The point is not that we shouldn't have them, but that we need to figure out ways to make this work nicely for users, and not force them to make copy/paste host and kernel versions of methods if we can help it...
sb10q commented 2021-02-17 09:01:43 +08:00
Owner
Copy Link

You could still use @portable, but outside kernel classes - see the example in #5 (comment)

You could still use ``@portable``, but outside kernel classes - see the example in https://git.m-labs.hk/M-Labs/nac3-spec/issues/5#issuecomment-1743
pca006132 commented 2021-03-30 09:54:50 +08:00
Collaborator
Copy Link

it seems like the proposed new artiq python scraps the concept of class attributes (which is fine since we're nowhere near the original python data model anyway) and uses the normal class attribute syntax to define type annotations for instance attributes. Is that about correct? Side note: I actually don't know how the current artiq-python treats class attributes.

I've checked PEP526#class-and-instance-variable-annotations, it seems that the syntax is valid python type annotation for instance variable.

One challenge is that it can be nice to write a method as @portable and then be able to run it on either the host or the kernel. This makes it easy to debug code by running it on the host, and then you can just plop the debugged code into a kernel. While this hides potential pitfalls, it is also nice to be able to do code reuse in this way.

Is there any comment about the alternative mentioned above?

> it seems like the proposed new artiq python scraps the concept of class attributes (which is fine since we're nowhere near the original python data model anyway) and uses the normal class attribute syntax to define type annotations for instance attributes. Is that about correct? Side note: I actually don't know how the current artiq-python treats class attributes. I've checked [PEP526#class-and-instance-variable-annotations](https://www.python.org/dev/peps/pep-0526/#class-and-instance-variable-annotations), it seems that the syntax is valid python type annotation for instance variable. > One challenge is that it can be nice to write a method as @portable and then be able to run it on either the host or the kernel. This makes it easy to debug code by running it on the host, and then you can just plop the debugged code into a kernel. While this hides potential pitfalls, it is also nice to be able to do code reuse in this way. Is there any comment about the alternative mentioned above?
hartytp commented 2021-03-30 15:37:50 +08:00
Author
Collaborator
Copy Link

I've checked PEP526#class-and-instance-variable-annotations, it seems that the syntax is valid python type annotation for instance variable.

Thanks for the reference. I don't really understand type annotations in python so that was interesting reading. I agree that your approach does seem to be the correct pythonic one.

The syntax here still makes me a little uncomfortable, but I think that's just python language mess (class attributes always feel like a massive footgun to me and these annotations make the situation more complex if anything) rather than an issue with this proposal.

> I've checked PEP526#class-and-instance-variable-annotations, it seems that the syntax is valid python type annotation for instance variable. Thanks for the reference. I don't really understand type annotations in python so that was interesting reading. I agree that your approach does seem to be the correct pythonic one. The syntax here still makes me a little uncomfortable, but I think that's just python language mess (class attributes always feel like a massive footgun to me and these annotations make the situation more complex if anything) rather than an issue with this proposal.
😆 1
pca006132 commented 2021-06-07 10:23:54 +08:00
Collaborator
Copy Link

Another issue that I just thought of: We use self.setattr_device all over the place in the current artiq code. Should we require the users to specify these instance attributes?

What if we set the attributes automatically when the users add the type annotations? For example:

from artiq.experiment import *


class BlinkForever(EnvExperiment):
    # some random type names...
    core: DeviceCore
    led0: DeviceLED

    @kernel
    def run(self):
        self.core.reset()
        while True:
            self.led0.pulse(100*ms)
            delay(100*ms)
Another issue that I just thought of: We use `self.setattr_device` all over the place in the current artiq code. Should we require the users to specify these instance attributes? What if we set the attributes automatically when the users add the type annotations? For example: ```python from artiq.experiment import * class BlinkForever(EnvExperiment): # some random type names... core: DeviceCore led0: DeviceLED @kernel def run(self): self.core.reset() while True: self.led0.pulse(100*ms) delay(100*ms) ```
sb10q commented 2021-06-07 10:31:34 +08:00
Owner
Copy Link

Yes we can sometimes set attributes automatically. The default build() method could call method that attempts to populate the devices from the type annotations and the device database.
It could be a simple match on the name of the device and the class described in the device database.

Yes we can sometimes set attributes automatically. The default ``build()`` method could call method that attempts to populate the devices from the type annotations and the device database. It could be a simple match on the name of the device and the class described in the device database.
pca006132 referenced this issue from a commit 2021-06-09 09:53:25 +08:00
Added #1 discussion
pca006132 referenced this issue from a commit 2021-06-09 10:00:08 +08:00
Added #1 discussion
lriesebos commented 2021-06-09 17:19:57 +08:00
Collaborator
Copy Link

I am not the biggest fan of "automatic population of attributes from the device db based on a type annotation". Like, should these devices be typed as KernelImmutable[TTLOut] or not, and when do you raise exceptions when there is a type mismatch or the key is not found? I find it a bit implicit, but if this is a desired feature and we can clarify how to type it and how the error-mechanisms work, I am fine with it.

Besides, I am wondering, will setattr_device() and get_device() stay? We mainly use the latter and in some cases the former. We find it very important that the key of the device does not have to be the same as the name of the attribute. And in case any of these two functions would be used, I assume they still need to be typed as KernelImmutable[TTLOut], so that could then conflict with automatically populating attributes from the device db.

I am not the biggest fan of "automatic population of attributes from the device db based on a type annotation". Like, should these devices be typed as `KernelImmutable[TTLOut]` or not, and when do you raise exceptions when there is a type mismatch or the key is not found? I find it a bit implicit, but if this is a desired feature and we can clarify how to type it and how the error-mechanisms work, I am fine with it. Besides, I am wondering, will `setattr_device()` and `get_device()` stay? We mainly use the latter and in some cases the former. We find it very important that the key of the device does not have to be the same as the name of the attribute. And in case any of these two functions would be used, I assume they still need to be typed as `KernelImmutable[TTLOut]`, so that could then conflict with automatically populating attributes from the device db.
sb10q commented 2021-06-09 17:22:08 +08:00
Owner
Copy Link

Sure, the current DDB functions will stay. And the automatic function is only in the default build() and does not get called when you overload build(), you'd have to call it yourself. It just makes simple experiments simpler.

Sure, the current DDB functions will stay. And the automatic function is only in the default ``build()`` and does not get called when you overload ``build()``, you'd have to call it yourself. It just makes simple experiments simpler.
lriesebos commented 2021-06-09 17:33:22 +08:00
Collaborator
Copy Link

Our inheritance stuctures are often quite complicated and we use super() in most of these classes to ensure all build() functions are called independent of the inheritance structure. Now, the last call to super() will result in a call to the empty build() function in ARTIQ, which is safe. With this change it would always call this new default function, which could then have conflicts. It would be great if there is some way to disable this default behavior (i.e. through a class variable AUTO_POPULATE_DEVICES=False) or to make it explicit (i.e. users need to call self.auto_populate_devices() to get this behavior.

For typing of device attributes, I have the impression that they should be KernelImmutable[TTLOut] to make them accessible from host code, but not sure if this automatic system should deal with that in a more flexible way.

Our inheritance stuctures are often quite complicated and we use `super()` in most of these classes to ensure all `build()` functions are called independent of the inheritance structure. Now, the last call to `super()` will result in a call to the empty `build()` function in ARTIQ, which is safe. With this change it would always call this new default function, which could then have conflicts. It would be great if there is some way to disable this default behavior (i.e. through a class variable `AUTO_POPULATE_DEVICES=False`) or to make it explicit (i.e. users need to call `self.auto_populate_devices()` to get this behavior. For typing of device attributes, I have the impression that they should be `KernelImmutable[TTLOut]` to make them accessible from host code, but not sure if this automatic system should deal with that in a more flexible way.
pca006132 commented 2021-06-09 17:36:57 +08:00
Collaborator
Copy Link

should these devices be typed as KernelImmutable[TTLOut] or not,

I think they should be typed as KernelImmutable[TTLOut], I forgot that they should be immutable kernel attributes.

and when do you raise exceptions when there is a type mismatch or the key is not found?

I think we could see if they are defined in the constructor. If they are, we silently ignore it (do not automatically populate it), otherwise provide a better error message (Use before definition, not defined automatically because there is no such key in the device database/with a different type).

Sure, the current DDB functions will stay. And the automatic function is only in the default build() and does not get called when you overload build(), you'd have to call it yourself. It just makes simple experiments simpler.

I'm OK with this.

> should these devices be typed as `KernelImmutable[TTLOut]` or not, I think they should be typed as `KernelImmutable[TTLOut]`, I forgot that they should be immutable kernel attributes. > and when do you raise exceptions when there is a type mismatch or the key is not found? I think we could see if they are defined in the constructor. If they are, we silently ignore it (do not automatically populate it), otherwise provide a better error message (Use before definition, not defined automatically because there is no such key in the device database/with a different type). > Sure, the current DDB functions will stay. And the automatic function is only in the default ``build()`` and does not get called when you overload ``build()``, you'd have to call it yourself. It just makes simple experiments simpler. I'm OK with this.
pca006132 commented 2021-06-10 10:13:48 +08:00
Collaborator
Copy Link

It would be great if there is some way to disable this default behavior (i.e. through a class variable AUTO_POPULATE_DEVICES=False) or to make it explicit (i.e. users need to call self.auto_populate_devices() to get this behavior.

Calling self.auto_populate_devices() looks good to me.

> It would be great if there is some way to disable this default behavior (i.e. through a class variable `AUTO_POPULATE_DEVICES=False`) or to make it explicit (i.e. users need to call `self.auto_populate_devices()` to get this behavior. Calling `self.auto_populate_devices()` looks good to me.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nac3-spec#1
No description provided.
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?