Kernel only attribute annotation (#76) #127
No reviewers
Labels
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#127
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "kernel_only_annotation"
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?
Kernel
annotation. When@nac3
decorator is applied to a class definition, it will find all the fields annotated withKernel
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 inKernel
and should have correct field access limitation. I now disablelist
to be inKernel
since they are not@nac3
and can be accessed and changed in the host without restriction. ButKernelInvariant
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.WIP: Kernel only attribute annotationto WIP: Kernel only attribute annotation (#76)@ -1075,0 +1076,4 @@
(slice, false)
}
_ => {
eprintln!("attributes not annotated with `Kernel` or `KernelInvariants` at {}", &annotation.location);
As before, please remove debug prints.
This is also not an error, if it is not annotated with
Kernel
orKernelInvariant
it is simply a host-only attribute.@ -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))
This is too restrictive. The restriction is worse than the possibility of bugs from the user mutating the list unexpectedly.
@ -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()) =>
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...@ -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")
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.
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 (thex
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 nowKernel[int]
,z
is nowKernelInvariant[int]
?).Ok, I think I will just remove this restriction on
list
inKernel
, likeKernelInvariant
?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.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__
"?The semantic is the same as original Python i.e. the type annotation is ignored by the interpreter itself.
Yes, we would rewrite the example as:
Yes.
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 insideKernel
orKernelInvariant
(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.Thanks, I think now I see clearer about the semantic of these annotations, will make changes accordingly.
Yes, current way is easy to implement and should be slightly faster than calling symbol resolver.
Yes I think essentially I am just overriding the python default
__setattr__
.To determine if the attribute is annotated with
Kernel
, I useimmutable_fields
(may be 'kernel_only_fields' will be a better wording for this?), which also takes account parent class' fields that are annotated withKernel
.For each
@nac3
class the check on__nac3_kernel_only_instances__
and theapply_kernel_only_constraints
is to prevent the interior mutability (something class' instance (which allows access from the host) assigned to be aKernel
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__
?Yes, so things like determining whether the code is in kernel is not implemented yet..
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 inmin_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..?Ok, then I think the the modifications would just be ignoring those fields not annotated with
Kernel
orKernelInvariant
in rust side? Since there is no more restrictions on kernel only variables in the rust side if I did not miss anything?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 inmin_artiq
.Instead of doing
You'd now be doing:
and ignore the former syntax (as far as NAC3 is concerned) since those are now host-only attributes.
It's a simple modification indeed.
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 becomeInvariant
in nac3standalone. Should also be easy to configure.Thanks for the above suggestions, they are implemented in the new commits.
WIP: Kernel only attribute annotation (#76)to Kernel only attribute annotation (#76)@ -9,6 +9,11 @@ use crate::{
use super::*;
pub enum CoreMode {
Just make the names configurable.
@ -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,
Replace with something like:
And they could default to non-ARTIQ
None / "Invariant"
.@ -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" }
You implemented
Default
already - don't repeat yourself?@ -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
Misleading comment. I suggest removing it, since it's fairly obvious what this does from the code already.
@ -379,0 +381,4 @@
let (mut composer, _, _) = TopLevelComposer::new(self.builtins.clone(), ComposerConfig {
kernel_ann: Some("Kernel"),
kernel_invariant_ann: "KernelInvariant"
});
We may want to refactor this at some point (store the composer object? store
ComposerConfig
?). But this can be a new patch.@ -243,0 +242,4 @@
let (_, builtins_def, builtins_ty) = TopLevelComposer::new(builtins.clone(), ComposerConfig {
kernel_ann: Some("Kernel"),
kernel_invariant_ann: "KernelInvariant"
});
duplication - see comment below
Removed in the latest commit.
Sorry that currently this constructing composer is indeed messy... will fix this when refactor composer.
Here we also want the
builtins_def
andbuiltins_ty
from constructing the composer usingnew
.. thedefault
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!I meant
Default
forComposerConfig
.Implemented in the new commmit.