Kernel only attribute annotation (#76) #127

Merged
sb10q merged 8 commits from kernel_only_annotation into master 2021-12-19 11:04:53 +08:00
Collaborator
  • Add Kernel annotation. When @nac3 decorator is applied to a class definition, it will find all the fields annotated with Kernel and prohibit the read and write to them after they are initialized in python

- Things not sure
- In the original discussion, 5 kinds of instance variable types are mentioned. And the original discussion also mentions about variables that are 'mutable when the kernel is not executed, and immutable when kernel is executing'. This PR currently only implement kernel only variables (not accessible at all after being initilized, regardless of whether the kernel is actually running or not (currently there is a flag allow_kernel_read, which is about allowing access to those kernel only fields during compilation since we would need these information))
- Regarding the list and interior mutability. Now custom classes with @nac3 can be in Kernel and should have correct field access limitation. I now disable list to be in Kernel since they are not @nac3 and can be accessed and changed in the host without restriction. But KernelInvariant seems to accept list to be annotated if I did not miss anything... So I am not sure whether I am handling list in a appropriate way.

- Add `Kernel` annotation. When `@nac3` decorator is applied to a class definition, it will find all the fields annotated with `Kernel` and prohibit the read and write to them after they are initialized in python --- ~~- **Things not sure** - In the original [discussion](https://git.m-labs.hk/M-Labs/nac3-spec/issues/5#issuecomment-2016), 5 kinds of instance variable types are mentioned. And the original discussion also mentions about variables that are 'mutable when the kernel is not executed, and immutable when kernel is executing'. This PR currently only implement kernel only variables (not accessible at all after being initilized, regardless of whether the kernel is actually running or not (currently there is a flag `allow_kernel_read`, which is about allowing access to those kernel only fields during compilation since we would need these information)) - Regarding the list and interior mutability. Now custom classes with `@nac3` can be in `Kernel` and should have correct field access limitation. I now disable `list` to be in `Kernel` since they are not `@nac3` and can be accessed and changed in the host without restriction. But `KernelInvariant` seems to accept list to be annotated if I did not miss anything... So I am not sure whether I am handling list in a appropriate way.~~
ychenfo added 2 commits 2021-12-10 17:13:43 +08:00
ychenfo changed title from WIP: Kernel only attribute annotation to WIP: Kernel only attribute annotation (#76) 2021-12-10 17:14:42 +08:00
sb10q reviewed 2021-12-11 09:50:14 +08:00
@ -1075,0 +1076,4 @@
(slice, false)
}
_ => {
eprintln!("attributes not annotated with `Kernel` or `KernelInvariants` at {}", &annotation.location);
Owner

As before, please remove debug prints.
This is also not an error, if it is not annotated with Kernel or KernelInvariant it is simply a host-only attribute.

As before, please remove debug prints. This is also not an error, if it is not annotated with ``Kernel`` or ``KernelInvariant`` it is simply a host-only attribute.
sb10q reviewed 2021-12-11 09:51:36 +08:00
@ -1071,0 +1066,4 @@
ast::ExprKind::Subscript { value, .. }
if matches!(&value.node, ast::ExprKind::Name { id, .. } if id == &"list".into()) =>
{
return Err(format!("list is not allowed to be `Kernel` at {}", value.location))
Owner

This is too restrictive. The restriction is worse than the possibility of bugs from the user mutating the list unexpectedly.

This is too restrictive. The restriction is worse than the possibility of bugs from the user mutating the list unexpectedly.
sb10q reviewed 2021-12-11 09:53:41 +08:00
@ -1070,1 +1061,3 @@
None
let (annotation, mutable) = match &annotation.node {
ast::ExprKind::Subscript { value, slice, .. }
if matches!(&value.node, ast::ExprKind::Name { id, .. } if id == &"Kernel".into()) =>
Owner

Again there is quite some inconsistency with match-by-name vs match-by-id, the virtual class seems to be to some extent match-by-id...

Again there is quite some inconsistency with match-by-name vs match-by-id, the ``virtual`` class seems to be to some extent match-by-id...
sb10q reviewed 2021-12-11 09:56:08 +08:00
@ -93,0 +113,4 @@
if not isclass(cls):
raise ValueError("nac3 annotation decorator should only be applied to classes")
if not cls.__setattr__ in {base.__setattr__ for base in cls.__bases__}:
raise ValueError("custom __setattr__ is not supported in kernel classes")
Owner

We renamed them "nac3 classes" to avoid confusion with things like "kernel methods" - a nac3 class can contain a mix of kernel and host items.

We renamed them "nac3 classes" to avoid confusion with things like "kernel methods" - a nac3 class can contain a mix of kernel and host items.
Author
Collaborator

This is also not an error, if it is not annotated with Kernel or KernelInvariant it is simply a host-only attribute.

If I am not mistaken, in the discussion, host-only attribute is simply those defined in the __init__ and not in the outer class definition body (the x in the example)? So I am not sure what the semantic should be for a variable not annotated with Kernel or KernelInvariant and defined in the class definition body (as I understand it, in the example, y is now Kernel[int], z is now KernelInvariant[int]?).

This is too restrictive. The restriction is worse than the possibility of bugs from the user mutating the list unexpectedly.

Ok, I think I will just remove this restriction on list in Kernel, like KernelInvariant?

Again there is quite some inconsistency with match-by-name vs match-by-id, the virtual class seems to be to some extent match-by-id...

Yes this is indeed some inconsistency... users can also do things like from artiq import Kernel as Ker... One way that I currently think of is that maybe we can later add a function to symbol resolver to resolve these 'special marker symbols' (I am not sure whether this is a good wording for this. But I think we need to add this function in symbol resolver trait because they are not always types in nac3core, just markers...). Then in nac3artiq we can match by id and in nac3standalone we can just match by string.

We renamed them "nac3 classes" to avoid confusion with things like "kernel methods" - a nac3 class can contain a mix of kernel and host items.

Oh I see, I will change from '..not supported in kernel classes' to '..not supported in nac3 classes'.

For the if not isclass(cls) check, I think it is still necessary since we can only @nac3 on classes?

For the custom __setattr__ check we can also loosen the restriction to "if the nac3 class do not contain any kernel only fields, users can be allowed to define their own __setattr__ and __getattribute__"?

> This is also not an error, if it is not annotated with Kernel or KernelInvariant it is simply a host-only attribute. If I am not mistaken, in the [discussion](https://git.m-labs.hk/M-Labs/nac3-spec/issues/5#issuecomment-2003), host-only attribute is simply those defined in the `__init__` and not in the outer class definition body (the `x` in the [example](https://git.m-labs.hk/M-Labs/nac3-spec/issues/5#issuecomment-2003))? So I am not sure what the semantic should be for a variable not annotated with Kernel or KernelInvariant and defined in the class definition body (as I understand it, in the [example](https://git.m-labs.hk/M-Labs/nac3-spec/issues/5#issuecomment-2003), `y` is now `Kernel[int]`, `z` is now `KernelInvariant[int]`?). > This is too restrictive. The restriction is worse than the possibility of bugs from the user mutating the list unexpectedly. Ok, I think I will just remove this restriction on `list` in `Kernel`, like `KernelInvariant`? > Again there is quite some inconsistency with match-by-name vs match-by-id, the virtual class seems to be to some extent match-by-id... Yes this is indeed some inconsistency... users can also do things like `from artiq import Kernel as Ker`... One way that I currently think of is that maybe we can later add a function to symbol resolver to resolve these 'special marker symbols' (I am not sure whether this is a good wording for this. But I think we need to add this function in symbol resolver trait because they are not always types in nac3core, just markers...). Then in nac3artiq we can match by id and in nac3standalone we can just match by string. > We renamed them "nac3 classes" to avoid confusion with things like "kernel methods" - a nac3 class can contain a mix of kernel and host items. Oh I see, I will change from '..not supported in kernel classes' to '..not supported in nac3 classes'. For the `if not isclass(cls)` check, I think it is still necessary since we can only `@nac3` on classes? For the custom `__setattr__` check we can also loosen the restriction to "if the nac3 class do not contain any kernel only fields, users can be allowed to define their own `__setattr__` and `__getattribute__`"?
Owner

So I am not sure what the semantic should be for a variable not annotated with Kernel or KernelInvariant and defined in the class definition body

The semantic is the same as original Python i.e. the type annotation is ignored by the interpreter itself.

(as I understand it, in the example, y is now Kernel[int], z is now KernelInvariant[int]?).

Yes, we would rewrite the example as:

  x: int  # optional
  y: Kernel[int]
  z: KernelInvariant[int]

Ok, I think I will just remove this restriction on list in Kernel, like KernelInvariant?

Yes.

Yes this is indeed some inconsistency... users can also do things like from artiq import Kernel as Ker...

We don't have to support all these corner cases if it makes things simpler, this is fine as long as it's documented and does not have obscure error messages. I just want things to be consistent, simple, and fast.

The Python code seems a bit complicated and I need to study it, I don't understand most of it currently. But I don't understand why not just define __setattr__ and raise an exception if the attribute is in the type annotations of the class and inside Kernel or KernelInvariant (with some boolean variable so it doesn't trigger in __init__). And this code is moot as long as we don't have RPC anyway.

> So I am not sure what the semantic should be for a variable not annotated with Kernel or KernelInvariant and defined in the class definition body The semantic is the same as original Python i.e. the type annotation is ignored by the interpreter itself. > (as I understand it, in the example, y is now Kernel[int], z is now KernelInvariant[int]?). Yes, we would rewrite the example as: ``` x: int # optional y: Kernel[int] z: KernelInvariant[int] ``` > Ok, I think I will just remove this restriction on list in Kernel, like KernelInvariant? Yes. > Yes this is indeed some inconsistency... users can also do things like from artiq import Kernel as Ker... We don't have to support all these corner cases if it makes things simpler, this is fine as long as it's documented and does not have obscure error messages. I just want things to be consistent, simple, and fast. The Python code seems a bit complicated and I need to study it, I don't understand most of it currently. But I don't understand why not just define ``__setattr__`` and raise an exception if the attribute is in the type annotations of the class and inside ``Kernel`` or ``KernelInvariant`` (with some boolean variable so it doesn't trigger in ``__init__``). And this code is moot as long as we don't have RPC anyway.
Author
Collaborator

Thanks, I think now I see clearer about the semantic of these annotations, will make changes accordingly.

We don't have to support all these corner cases if it makes things simpler, this is fine as long as it's documented and does not have obscure error messages. I just want things to be consistent, simple, and fast.

Yes, current way is easy to implement and should be slightly faster than calling symbol resolver.

why not just define __setattr__ and raise an exception if the attribute is in the type annotations of the class and inside Kernel or KernelInvariant (with some boolean variable so it doesn't trigger in __init__).

Yes I think essentially I am just overriding the python default __setattr__.

To determine if the attribute is annotated with Kernel, I use immutable_fields(may be 'kernel_only_fields' will be a better wording for this?), which also takes account parent class' fields that are annotated with Kernel.

For each @nac3 class the check on __nac3_kernel_only_instances__ and the apply_kernel_only_constraints is to prevent the interior mutability (something class' instance (which allows access from the host) assigned to be a Kernel field).

I have not came up with a better idea for detecting whether we are exactly in __init__ for now... I just check whether the field exists or not. I might missed something.. I am not sure how to implicitly set a bool flag indicating that we are in __init__?

And this code is moot as long as we don't have RPC anyway.

Yes, so things like determining whether the code is in kernel is not implemented yet..

Thanks, I think now I see clearer about the semantic of these annotations, will make changes accordingly. > We don't have to support all these corner cases if it makes things simpler, this is fine as long as it's documented and does not have obscure error messages. I just want things to be consistent, simple, and fast. Yes, current way is easy to implement and should be slightly faster than calling symbol resolver. > why not just define `__setattr__` and raise an exception if the attribute is in the type annotations of the class and inside `Kernel` or `KernelInvariant` (with some boolean variable so it doesn't trigger in `__init__`). Yes I think essentially I am just overriding the python default `__setattr__`. To determine if the attribute is annotated with `Kernel`, I use `immutable_fields`(may be 'kernel_only_fields' will be a better wording for this?), which also takes account parent class' fields that are annotated with `Kernel`. For each `@nac3` class the check on `__nac3_kernel_only_instances__` and the `apply_kernel_only_constraints` is to prevent the interior mutability (something class' instance (which allows access from the host) assigned to be a `Kernel` field). I have not came up with a better idea for detecting whether we are exactly in `__init__` for now... I just check whether the field exists or not. I might missed something.. I am not sure how to implicitly set a bool flag indicating that we are in `__init__`? > And this code is moot as long as we don't have RPC anyway. Yes, so things like determining whether the code is in kernel is not implemented yet..
Owner

I am not sure how to implicitly set a bool flag indicating that we are in init?

Currently we are always in __init__ since there is no RPC.

Maybe you can just ignore the Python-side restrictions for this repos, which are not necessary for min_artiq and would involve things like the core device driver. No need to copy/reimplement too many things in ARTIQ and in min_artiq.

> I am not sure how to implicitly set a bool flag indicating that we are in __init__? Currently we are always in ``__init__`` since there is no RPC. Maybe you can just ignore the Python-side restrictions for this repos, which are not necessary for ``min_artiq`` and would involve things like the core device driver. No need to copy/reimplement too many things in ARTIQ and in ``min_artiq``.
Author
Collaborator

Currently we are always in __init__ since there is no RPC.

Sorry I think I do not quite get what you mean here, now in nac3artiq we are also outside of __init__ when we are not constructing an @nac3 object..?

Maybe you can just ignore the Python-side restrictions for this repos, which are not necessary for min_artiq and would involve things like the core device driver. No need to copy/reimplement too many things in ARTIQ and in min_artiq.

Ok, then I think the the modifications would just be ignoring those fields not annotated with Kernel or KernelInvariant in rust side? Since there is no more restrictions on kernel only variables in the rust side if I did not miss anything?

> Currently we are always in `__init__` since there is no RPC. Sorry I think I do not quite get what you mean here, now in nac3artiq we are also outside of `__init__` when we are not constructing an `@nac3` object..? > Maybe you can just ignore the Python-side restrictions for this repos, which are not necessary for min_artiq and would involve things like the core device driver. No need to copy/reimplement too many things in ARTIQ and in min_artiq. Ok, then I think the the modifications would just be ignoring those fields not annotated with `Kernel` or `KernelInvariant` in rust side? Since there is no more restrictions on kernel only variables in the rust side if I did not miss anything?
Owner

Sorry I think I do not quite get what you mean here, now in nac3artiq we are also outside of init when we are not constructing an @nac3 object..?

If it's not a nac3 object then there is no Kernel attribute.

Anyway let me do the Python attribute enforcement code, just focus this PR on adding Kernel. As suggested I wouldn't bother enforcing in min_artiq.

> Sorry I think I do not quite get what you mean here, now in nac3artiq we are also outside of __init__ when we are not constructing an @nac3 object..? If it's not a nac3 object then there is no ``Kernel`` attribute. Anyway let me do the Python attribute enforcement code, just focus this PR on adding ``Kernel``. As suggested I wouldn't bother enforcing in ``min_artiq``.
Owner

Ok, then I think the the modifications would just be ignoring those fields not annotated with Kernel or KernelInvariant in rust side?

Instead of doing

x: int32

You'd now be doing:

x: Kernel[int32]

and ignore the former syntax (as far as NAC3 is concerned) since those are now host-only attributes.

It's a simple modification indeed.

> Ok, then I think the the modifications would just be ignoring those fields not annotated with Kernel or KernelInvariant in rust side? Instead of doing ``` x: int32 ``` You'd now be doing: ``` x: Kernel[int32] ``` and ignore the former syntax (as far as NAC3 is concerned) since those are now host-only attributes. It's a simple modification indeed.
Owner

And you can make Kernel optional with a flag in nac3core since there is no notion of host-only attribute in nac3standalone. KernelInvariant could also become Invariant in nac3standalone. Should also be easy to configure.

And you can make ``Kernel`` optional with a flag in nac3core since there is no notion of host-only attribute in nac3standalone. ``KernelInvariant`` could also become ``Invariant`` in nac3standalone. Should also be easy to configure.
ychenfo added 3 commits 2021-12-15 02:29:44 +08:00
Author
Collaborator

Thanks for the above suggestions, they are implemented in the new commits.

Thanks for the above suggestions, they are implemented in the new commits.
ychenfo changed title from WIP: Kernel only attribute annotation (#76) to Kernel only attribute annotation (#76) 2021-12-15 02:33:31 +08:00
sb10q reviewed 2021-12-15 11:09:19 +08:00
@ -9,6 +9,11 @@ use crate::{
use super::*;
pub enum CoreMode {
Owner

Just make the names configurable.

Just make the names configurable.
sb10q reviewed 2021-12-15 11:10:22 +08:00
@ -26,2 +31,4 @@
// number of built-in function and classes in the definition list, later skip
pub builtin_num: usize,
// indicate the mode that we are using the core
pub mode: CoreMode,
Owner

Replace with something like:

pub kernel_ann: Option<str>,
pub kernel_invariant_ann: str
Replace with something like: ``` pub kernel_ann: Option<str>, pub kernel_invariant_ann: str ```
Owner

And they could default to non-ARTIQ None / "Invariant".

And they could default to non-ARTIQ ``None / "Invariant"``.
ychenfo added 1 commit 2021-12-17 04:44:29 +08:00
sb10q reviewed 2021-12-17 12:46:09 +08:00
@ -50,1 +47,3 @@
let (mut composer, builtins_def, builtins_ty) = TopLevelComposer::new(vec![]);
let (mut composer, builtins_def, builtins_ty) = TopLevelComposer::new(
vec![],
ComposerConfig { kernel_ann: None, kernel_invariant_ann: "Invariant" }
Owner

You implemented Default already - don't repeat yourself?

You implemented ``Default`` already - don't repeat yourself?
sb10q reviewed 2021-12-17 12:47:37 +08:00
@ -25,11 +30,16 @@ pub struct TopLevelComposer {
pub method_class: HashMap<DefinitionId, DefinitionId>,
// number of built-in function and classes in the definition list, later skip
pub builtin_num: usize,
// indicate the mode that we are using the core
Owner

Misleading comment. I suggest removing it, since it's fairly obvious what this does from the code already.

Misleading comment. I suggest removing it, since it's fairly obvious what this does from the code already.
sb10q reviewed 2021-12-17 12:50:06 +08:00
@ -379,0 +381,4 @@
let (mut composer, _, _) = TopLevelComposer::new(self.builtins.clone(), ComposerConfig {
kernel_ann: Some("Kernel"),
kernel_invariant_ann: "KernelInvariant"
});
Owner

We may want to refactor this at some point (store the composer object? store ComposerConfig?). But this can be a new patch.

We may want to refactor this at some point (store the composer object? store ``ComposerConfig``?). But this can be a new patch.
sb10q reviewed 2021-12-17 12:50:29 +08:00
@ -243,0 +242,4 @@
let (_, builtins_def, builtins_ty) = TopLevelComposer::new(builtins.clone(), ComposerConfig {
kernel_ann: Some("Kernel"),
kernel_invariant_ann: "KernelInvariant"
});
Owner

duplication - see comment below

duplication - see comment below
ychenfo added 1 commit 2021-12-18 04:54:51 +08:00
Author
Collaborator

Misleading comment. I suggest removing it, since it's fairly obvious what this does from the code already.

Removed in the latest commit.

We may want to refactor this at some point (store the composer object? store ComposerConfig?). But this can be a new patch.
duplication - see comment below

Sorry that currently this constructing composer is indeed messy... will fix this when refactor composer.

You implemented Default already - don't repeat yourself?

Here we also want the builtins_def and builtins_ty from constructing the composer using new.. the default will not return these.. so I did not fix this in the latest commit - But this is also indeed messy, will try to fix this when refactor composer. Thanks for pointing this out!

> Misleading comment. I suggest removing it, since it's fairly obvious what this does from the code already. Removed in the latest commit. > We may want to refactor this at some point (store the composer object? store ComposerConfig?). But this can be a new patch. > duplication - see comment below Sorry that currently this constructing composer is indeed messy... will fix this when refactor composer. > You implemented Default already - don't repeat yourself? Here we also want the `builtins_def` and `builtins_ty` from constructing the composer using `new`.. the `default` will not return these.. so I did not fix this in the latest commit - But this is also indeed messy, will try to fix this when refactor composer. Thanks for pointing this out!
Owner

I meant Default for ComposerConfig.

I meant ``Default`` for ``ComposerConfig``.
ychenfo added 1 commit 2021-12-19 02:55:46 +08:00
Author
Collaborator

I meant Default for ComposerConfig.

Implemented in the new commmit.

> I meant ``Default`` for ``ComposerConfig``. Implemented in the new commmit.
sb10q merged commit 91625dd327 into master 2021-12-19 11:04:53 +08:00
sb10q deleted branch kernel_only_annotation 2021-12-19 11:04:53 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 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#127
No description provided.