M-Labs/nac3
M-Labs/nac3
8
1
Fork 12
Code Issues 98 Pull Requests 19 Releases Wiki Activity

core: allow calling parent methods #497

Merged
sb10q merged 4 commits from issue-133 into master 2024-08-26 18:37:55 +08:00
Conversation 6 Commits 4 Files Changed 5 +287 -28
abdul124 commented 2024-08-19 10:46:33 +08:00
Collaborator
Copy Link

This PR adds support for calling other class or ancestor functions as discussed in #133. It also adds capability to call parent functions #136 (the super keyword is not declared but the functionality is achieved by specifying class names).

Initialization of variables can be done as:

class Foo(Bar):
  def __init__(self):
    Bar.__init__(self)
    self.init()

  def init(self):
    pass

However, unlike Python when overriding, the function signature must be same. Moreover calling class functions with self is already mapped to the corresponding class function definition IDs, this means that:

class Foo:
  def f1(self):
    self.f2()
  def f2(self):
    output_int32(1)

class Bar(Foo):
  def f2(self):
    output_int32(2)
 def f3(self):
   self.f1()

will output 1 in nac3 and 2 in Python. The Python behavior can be supported by evaluating the definitionIDs of the function on each call, but again this is because of Python's lax type system and in other languages like C++ this behaves similar to self in nac3. I will create another PR to support the virtual polymorphism if its needed.
The tests have been added to nac3standalone/demo/src/inheritance.py

Fixes #133.

This PR adds support for calling other class or ancestor functions as discussed in #133. It also adds capability to call parent functions #136 (the `super` keyword is not declared but the functionality is achieved by specifying class names). Initialization of variables can be done as: ``` class Foo(Bar): def __init__(self): Bar.__init__(self) self.init() def init(self): pass ``` However, unlike Python when overriding, the function signature must be same. Moreover calling class functions with `self` is already mapped to the corresponding class function definition IDs, this means that: ``` class Foo: def f1(self): self.f2() def f2(self): output_int32(1) class Bar(Foo): def f2(self): output_int32(2) def f3(self): self.f1() ``` will output 1 in nac3 and 2 in Python. The Python behavior can be supported by evaluating the definitionIDs of the function on each call, but again this is because of Python's lax type system and in other languages like C++ `this` behaves similar to `self` in nac3. I will create another PR to support the virtual polymorphism if its needed. The tests have been added to `nac3standalone/demo/src/inheritance.py` Fixes #133.
sb10q reviewed 2024-08-19 13:09:04 +08:00
nac3core/src/toplevel/helper.rs Outdated
@ -791,1 +814,3 @@
result.extend(Self::get_all_assigned_field(body.as_slice())?);
result.extend(Self::get_all_assigned_field(class_name, ast, body.as_slice())?);
}
// Variables Initiated in function calls
sb10q commented 2024-08-19 13:09:04 +08:00
Owner
Copy Link

Initiated ?

Initiated ?
abdul124 commented 2024-08-19 13:30:11 +08:00
Author
Collaborator
Copy Link

Initiated ?

Should have been initialized. This part checks if there are any function calls in the __init__ block where user might have initialized some variables (as all variables must be initialized before creating an object).

> Initiated ? Should have been initialized. This part checks if there are any function calls in the `__init__` block where user might have initialized some variables (as all variables must be initialized before creating an object).
sb10q reviewed 2024-08-19 13:10:04 +08:00
nac3core/src/toplevel/composer.rs Outdated
@ -1823,3 +1823,3 @@
unreachable!("must be init function here")
}
let all_inited = Self::get_all_assigned_field(body.as_slice())?;
// Since AST stores class names without prepending `__module__.`, we split the name for search purposes
sb10q commented 2024-08-19 13:10:04 +08:00
Owner
Copy Link

I don't understand this comment, can you elaborate?
Is a string hack the best way of solving this?

I don't understand this comment, can you elaborate? Is a string hack the best way of solving this?
abdul124 commented 2024-08-19 13:22:51 +08:00
Author
Collaborator
Copy Link

I don't understand this comment, can you elaborate?

Class names in TopLevelDef are stored by first prepending module name to the class name. So,

class A:
   pass

would be named __main__.A. However, in the raw AST that holds the statements, the name is just A, so here I am removing the module name from definition.

Is a string hack the best way of solving this?

Hmm this was the most straightforward method. An alternative could be to have to do the search in the TopLevelDef and get the corresponding DefinitionID to fetch the function statements from the AST.

> I don't understand this comment, can you elaborate? Class names in TopLevelDef are stored by first prepending module name to the class name. So, ``` class A: pass ``` would be named \_\_main\_\_.A. However, in the raw AST that holds the statements, the name is just A, so here I am removing the module name from definition. > Is a string hack the best way of solving this? Hmm this was the most straightforward method. An alternative could be to have to do the search in the TopLevelDef and get the corresponding DefinitionID to fetch the function statements from the AST.
sb10q commented 2024-08-19 14:39:01 +08:00
Owner
Copy Link

Seems to me that doing the alternative method would make it clear what is happening from the code alone, and would not need any of those obscure/complicated comments.

Seems to me that doing the alternative method would make it clear what is happening from the code alone, and would not need any of those obscure/complicated comments.
sb10q requested review from derppening 2024-08-19 13:11:28 +08:00
derppening reviewed 2024-08-19 14:47:58 +08:00
derppening left a comment
Collaborator
Copy Link

Please add "Fixes #133" to the end of the PR message to make it explicitly clear this PR closes that issue.

Have you tried this with an ARTIQ kernel?

Please add "Fixes #133" to the end of the PR message to make it explicitly clear this PR closes that issue. Have you tried this with an ARTIQ kernel?
nac3core/src/toplevel/composer.rs Outdated
@ -1826,0 +1828,4 @@
class_name_only.split_once('.').unwrap_or(("", &class_name_only));
let all_inited = Self::get_all_assigned_field(
class_name_only.into(),
derppening commented 2024-08-19 14:38:31 +08:00
Collaborator
Copy Link

Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes?

Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes?
abdul124 commented 2024-08-19 15:32:25 +08:00
Author
Collaborator
Copy Link

Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes?

Hmm I tried testing on inner classes, but they are not currently supported in nac3. Also, by matching name against the class names in the TopLevelDef as suggested in #497 (comment) we can avoid the problem altogether.

> Would this way of getting the fully qualified name of the to-be-initialized class work for inner (nested) classes? Hmm I tried testing on inner classes, but they are not currently supported in nac3. Also, by matching name against the class names in the TopLevelDef as suggested in https://git.m-labs.hk/M-Labs/nac3/pulls/497#issuecomment-11572 we can avoid the problem altogether.
nac3core/src/toplevel/helper.rs Outdated
@ -734,3 +735,3 @@
}
pub fn get_all_assigned_field(stmts: &[Stmt<()>]) -> Result<HashSet<StrRef>, HashSet<String>> {
pub fn get_all_assigned_field(
derppening commented 2024-08-19 14:40:20 +08:00
Collaborator
Copy Link

Could you add documentation to this function?

Could you add documentation to this function?
abdul124 commented 2024-08-19 15:32:41 +08:00
Author
Collaborator
Copy Link

Could you add documentation to this function?

Will do.

> Could you add documentation to this function? Will do.
nac3core/src/toplevel/helper.rs Outdated
@ -792,0 +824,4 @@
let ExprKind::Name { id, .. } = &value.node else {
continue;
};
// Need to conside the two cases:
derppening commented 2024-08-19 14:39:37 +08:00
Collaborator
Copy Link

NIt: consider

NIt: consider
nac3core/src/typecheck/type_inferencer/mod.rs Outdated
@ -1675,0 +1678,4 @@
/// Returns [`None`] if its not a call to parent method, otherwise
/// returns a new `func` with class name replaced by `self` and method resolved to its `DefinitionID`
///
/// e.g. A.f1(self, ...) returns Some(self.DefintionID(f1))
derppening commented 2024-08-19 14:44:55 +08:00
Collaborator
Copy Link

Do you mean it returns Some(DefinitionID(self.f1))? Aren't DefinitionIDs global?

Do you mean it returns `Some(DefinitionID(self.f1))`? Aren't `DefinitionID`s global?
abdul124 commented 2024-08-19 15:19:49 +08:00
Author
Collaborator
Copy Link

Do you mean it returns Some(DefinitionID(self.f1))? Aren't DefinitionIDs global?

Since each class function is assigned a unique DefintionID, this returns replaces the attribute with the definitionID of the actual function to be called. To illustrate:

class Foo:
  def f1(self):
    pass
class Bar(Foo):
  def f1(self):
    Foo.f1(self)

Here Foo.f1() and Bar.f1() will be assigned different Ids say 1 and 2. The call to Foo.f1(self) will be changed by self.1(). Under normal execution when generating function call in gen_expr, a lookup is done on the class definition to find the definitionId of the corresponding method. Here, I am doing the lookup beforehand to point to the correct method.

> Do you mean it returns `Some(DefinitionID(self.f1))`? Aren't `DefinitionID`s global? Since each class function is assigned a unique DefintionID, this returns replaces the attribute with the definitionID of the actual function to be called. To illustrate: ``` class Foo: def f1(self): pass class Bar(Foo): def f1(self): Foo.f1(self) ``` Here `Foo.f1()` and `Bar.f1() `will be assigned different Ids say 1 and 2. The call to `Foo.f1(self)` will be changed by `self.1()`. Under normal execution when generating function call in `gen_expr`, a lookup is done on the class definition to find the definitionId of the corresponding method. Here, I am doing the lookup beforehand to point to the correct method.
derppening commented 2024-08-19 15:34:36 +08:00
Collaborator
Copy Link

Could you wrap DefinitionID with some symbol (e.g. self.{DefinitionID(f1)}) to indicate that the returned DefinitionID is supposed to be substituted by the function returns?

Could you wrap `DefinitionID` with some symbol (e.g. `self.{DefinitionID(f1)}`) to indicate that the returned `DefinitionID` is supposed to be substituted by the function returns?
👍 1
abdul124 force-pushed issue-133 from adadbaa6ba to 79e95653cd 2024-08-19 17:55:12 +08:00 Compare
abdul124 force-pushed issue-133 from 79e95653cd to 415c78d23b 2024-08-20 12:26:12 +08:00 Compare
abdul124 commented 2024-08-20 12:35:11 +08:00
Author
Collaborator
Copy Link

Have you tried this with an ARTIQ kernel?

After testing with ARTIQ kernels, the polymorphism is working when functions of both classes are defined to be on same side (i.e. either both are @kernel or @rpc). So, this is currently breaking:

@nac3
class A:
    a: Kernel[int32]
    
    def __init__(self, a: int32):
        self.a = a
    
    # @kernel
    def output_all_fields(self):
        raise ZeroDivisionError
    
    @kernel
    def set_a(self, a: int32):
        self.a = a

@nac3
class B(A, EnvExperiment):
    def build(self):
        self.setattr_device("core")
    
    def __init__(self, managers_or_parent, a: int32):
        EnvExperiment.__init__(self, managers_or_parent)
        A.__init__(self, a)
    
    @kernel
    def output_all_fields(self):
        A.output_all_fields(self)

The reason for this is that during the TopLevelDef during type checking for the @kernel methods does not have definitions of other non kernel functions and so is unable to find output_all_fields in class A.

> Have you tried this with an ARTIQ kernel? After testing with ARTIQ kernels, the polymorphism is working when functions of both classes are defined to be on same side (i.e. either both are `@kernel` or `@rpc`). So, this is currently breaking: ``` @nac3 class A: a: Kernel[int32] def __init__(self, a: int32): self.a = a # @kernel def output_all_fields(self): raise ZeroDivisionError @kernel def set_a(self, a: int32): self.a = a @nac3 class B(A, EnvExperiment): def build(self): self.setattr_device("core") def __init__(self, managers_or_parent, a: int32): EnvExperiment.__init__(self, managers_or_parent) A.__init__(self, a) @kernel def output_all_fields(self): A.output_all_fields(self) ``` The reason for this is that during the `TopLevelDef` during type checking for the `@kernel` methods does not have definitions of other non kernel functions and so is unable to find `output_all_fields` in class `A`.
derppening commented 2024-08-20 20:03:24 +08:00
Collaborator
Copy Link

Have you tried this with an ARTIQ kernel?

After testing with ARTIQ kernels, the polymorphism is working when functions of both classes are defined to be on same side (i.e. either both are @kernel or @rpc). So, this is currently breaking:

@nac3
class A:
    a: Kernel[int32]
    
    def __init__(self, a: int32):
        self.a = a
    
    # @kernel
    def output_all_fields(self):
        raise ZeroDivisionError
    
    @kernel
    def set_a(self, a: int32):
        self.a = a

@nac3
class B(A, EnvExperiment):
    def build(self):
        self.setattr_device("core")
    
    def __init__(self, managers_or_parent, a: int32):
        EnvExperiment.__init__(self, managers_or_parent)
        A.__init__(self, a)
    
    @kernel
    def output_all_fields(self):
        A.output_all_fields(self)

The reason for this is that during the TopLevelDef during type checking for the @kernel methods does not have definitions of other non kernel functions and so is unable to find output_all_fields in class A.

What is the error message emitted here?

> > Have you tried this with an ARTIQ kernel? > > After testing with ARTIQ kernels, the polymorphism is working when functions of both classes are defined to be on same side (i.e. either both are `@kernel` or `@rpc`). So, this is currently breaking: > > ``` > @nac3 > class A: > a: Kernel[int32] > > def __init__(self, a: int32): > self.a = a > > # @kernel > def output_all_fields(self): > raise ZeroDivisionError > > @kernel > def set_a(self, a: int32): > self.a = a > > @nac3 > class B(A, EnvExperiment): > def build(self): > self.setattr_device("core") > > def __init__(self, managers_or_parent, a: int32): > EnvExperiment.__init__(self, managers_or_parent) > A.__init__(self, a) > > @kernel > def output_all_fields(self): > A.output_all_fields(self) > ``` > The reason for this is that during the `TopLevelDef` during type checking for the `@kernel` methods does not have definitions of other non kernel functions and so is unable to find `output_all_fields` in class `A`. What is the error message emitted here?
abdul124 commented 2024-08-20 20:36:03 +08:00
Author
Collaborator
Copy Link

What is the error message emitted here?

The error is from

nac3core/src/typecheck/type_inferencer/mod.rs Line 1750 in 415c78d23b
None => report_error(

The error is raised since the function (A.output_all_fields) was not marked with one of the decorators (rpc, kernel, portable), its definition is ignored and is not part of the definition list at
nac3core/src/typecheck/type_inferencer/mod.rs Line 1706 in 415c78d23b
let defs = self.top_level.definitions.read();
. I will look into whether we can make it work by declaring it as portable or rpc.

I am not too sure if we want to support this way of calling functions though. Since we are enforcing signatures to be same for overridden functions, wouldn't it be better to add check to ensure they are declared with same decorators as well.

> What is the error message emitted here? The error is from https://git.m-labs.hk/M-Labs/nac3/src/commit/415c78d23b710dc8ea0b49993f2cb8dcda3a2102/nac3core/src/typecheck/type_inferencer/mod.rs#L1750 The error is raised since the function (`A.output_all_fields`) was not marked with one of the decorators (`rpc`, `kernel`, `portable`), its definition is ignored and is not part of the definition list at https://git.m-labs.hk/M-Labs/nac3/src/commit/415c78d23b710dc8ea0b49993f2cb8dcda3a2102/nac3core/src/typecheck/type_inferencer/mod.rs#L1706. I will look into whether we can make it work by declaring it as `portable` or `rpc`. I am not too sure if we want to support this way of calling functions though. Since we are enforcing signatures to be same for overridden functions, wouldn't it be better to add check to ensure they are declared with same decorators as well.
abdul124 added 1 commit 2024-08-22 16:44:59 +08:00
derppening commented 2024-08-26 14:30:57 +08:00
Collaborator
Copy Link

Please check that this also works with ARTIQ.

Please check that this also works with ARTIQ.
abdul124 commented 2024-08-26 15:56:11 +08:00
Author
Collaborator
Copy Link

Please check that this also works with ARTIQ.

from artiq.experiment import *
from artiq.coredevice.core import Core
from numpy import int32

@nac3
class A:
    a: Kernel[int32]
    def __init__(self, a: int32):
        self.a = a
    
    @rpc
    def output_all_fields(self):
        print(self.a)

    @portable
    def set_a(self, a: int32):
        self.a = a

@nac3
class B(A):
    b: Kernel[int32]

    def __init__(self, b: int32):
        A.__init__(self, b + 1)
        self.set_b(b)
    
    @kernel
    def output_parent_fields(self):
        A.output_all_fields(self)

    @rpc
    def output_all_fields(self):
        A.output_all_fields(self)
        print(self.b)
    
    @portable
    def set_b(self, b: int32):
        self.b = b

@nac3
class C(B, EnvExperiment):
    c: Kernel[int32]
    core: KernelInvariant[Core]

    def build(self):
        
        self.setattr_device("core")

    def __init__(self, others):
        EnvExperiment.__init__(self, others)
        self.c = 1
        B.__init__(self, self.c + 1)
    
    @kernel
    def output_parent_fields(self):
        B.output_all_fields(self)

    @rpc
    def output_all_fields(self):
        B.output_all_fields(self)
        print(self.c)
    
    @kernel
    def set_c(self, c: int32):
        self.c = c
    
    @rpc
    def run(self):
        # Display Fields
        self.output_all_fields()
        self.output_parent_fields()

        # Display only fields from B
        B.output_all_fields(self)

        # Modify Fields
        A.set_a(self, 10)
        B.set_b(self, 11)
        self.set_c(12)

        print_rpc(self.a)
        print_rpc(self.b)
        print_rpc(self.c)

I have tested the changes on ARTIQ (nac3 branch) and the polymorphism is working as expected there as well. The above code will output:

3
2
1
3
2
3
2
10
11
12
> Please check that this also works with ARTIQ. ``` from artiq.experiment import * from artiq.coredevice.core import Core from numpy import int32 @nac3 class A: a: Kernel[int32] def __init__(self, a: int32): self.a = a @rpc def output_all_fields(self): print(self.a) @portable def set_a(self, a: int32): self.a = a @nac3 class B(A): b: Kernel[int32] def __init__(self, b: int32): A.__init__(self, b + 1) self.set_b(b) @kernel def output_parent_fields(self): A.output_all_fields(self) @rpc def output_all_fields(self): A.output_all_fields(self) print(self.b) @portable def set_b(self, b: int32): self.b = b @nac3 class C(B, EnvExperiment): c: Kernel[int32] core: KernelInvariant[Core] def build(self): self.setattr_device("core") def __init__(self, others): EnvExperiment.__init__(self, others) self.c = 1 B.__init__(self, self.c + 1) @kernel def output_parent_fields(self): B.output_all_fields(self) @rpc def output_all_fields(self): B.output_all_fields(self) print(self.c) @kernel def set_c(self, c: int32): self.c = c @rpc def run(self): # Display Fields self.output_all_fields() self.output_parent_fields() # Display only fields from B B.output_all_fields(self) # Modify Fields A.set_a(self, 10) B.set_b(self, 11) self.set_c(12) print_rpc(self.a) print_rpc(self.b) print_rpc(self.c) ``` I have tested the changes on ARTIQ (nac3 branch) and the polymorphism is working as expected there as well. The above code will output: ``` 3 2 1 3 2 3 2 10 11 12 ```
derppening commented 2024-08-26 17:36:26 +08:00
Collaborator
Copy Link

No further comments.

No further comments.
derppening requested review from sb10q 2024-08-26 17:36:30 +08:00
sb10q merged commit 5b2b6db7ed into master 2024-08-26 18:37:55 +08:00
sb10q deleted branch issue-133 2024-08-26 18:37:56 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
derppening
sb10q
Labels
Clear labels   
future-extension

   
high-priority

   
low-priority

   
UROP0622
No Label
future-extension
high-priority
low-priority
UROP0622
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 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#497
No description provided.
Delete Branch "issue-133"

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?