core: allow calling parent methods #497

abdul124 · 2024-08-19T10:46:33+08:00

abdul124 commented

2024-08-19 10:46:33 +08:00

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.

abdul124 added 3 commits 2024-08-19 10:46:33 +08:00

76c249bdb8 core: allow Call and AnnAssign in init block

3771255e06 core: add support for simple polymorphism

adadbaa6ba standalone: add tests for polymorphism

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

Initiated ?

abdul124 commented

2024-08-19 13:30:11 +08:00

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

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

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

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

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

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

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

Could you add documentation to this function?

abdul124 commented

2024-08-19 15:32:41 +08:00

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

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

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

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

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

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

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

What is the error message emitted here?

The error is from 415c78d23b/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 415c78d23b/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.

> 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

d26c75837f core: improve error messages

derppening commented

2024-08-26 14:30:57 +08:00

Please check that this also works with ARTIQ.

abdul124 commented

2024-08-26 15:56:11 +08:00

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:

> 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

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.

No reviewers