Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler doesn't parse @PREFIX.kernel (kernel decorator with path) #1162

Open
drewrisinger opened this issue Sep 27, 2018 · 20 comments · Fixed by #1710
Open

Compiler doesn't parse @PREFIX.kernel (kernel decorator with path) #1162

drewrisinger opened this issue Sep 27, 2018 · 20 comments · Fixed by #1710

Comments

@drewrisinger
Copy link
Contributor

drewrisinger commented Sep 27, 2018

Description

ARTIQ compiler doesn't recognize @PREFIX.kernel decorator.

Replicating

Instead of using from artiq.experiments import *, use import artiq.language.core. Then decorate function with @artiq.language.core.kernel.

Sample Code

I would like to use following, because per PEP20, Explicit is better than implicit. Also, because it avoids cluttering the namespace and speeds imports.

Sample based on LED tutorial:

import artiq.language.environment as artiq_env
import artiq.language.core as artiq_lang_core

class LED(artiq_env.EnvExperiment):
    def build(self):
        self.setattr_device("core")
        self.setattr_device("led")

    @artiq_lang_core.kernel
    def run(self):
        self.core.reset()
        self.led.on()

Behavior

Expected Behavior

ARTIQ compiler should compile the function for the core device as it would any other function with @kernel

Actual Behavior

ARTIQ throws CompileError (when run in separate pytest config), or standard error with above sample & artiq_run. Seems to be issue with

def visit_FunctionDefT(self, node):
for index, decorator in enumerate(node.decorator_list):
if types.is_builtin(decorator.type, "kernel") or \
isinstance(decorator, asttyped.CallT) and \
types.is_builtin(decorator.func.type, "kernel"):
continue
diag = diagnostic.Diagnostic("error",
"decorators are not supported", {},
node.at_locs[index], [])
self.engine.process(diag)

Tested also with import artiq.language.core; @artiq.language.core.kernel, and produced same result.

Log Message/Traceback

With artiq_run: error: decorators are not supported @artiq_lang_core.kernel.

Full traceback

$ artiq_run ./led_kernel_explicit.py --vv
DEBUG:artiq.compiler.import_cache: hook installed
DEBUG:artiq.compiler.import_cache: added 'PATH/artiq/coredevice/ttl.py' to cache
./led_kernel_explicit.py:9:5-9:6: error: decorators are not supported
    @artiq_lang_core.kernel
    ^
@jordens
Copy link
Member

jordens commented Sep 27, 2018

I agree. The star imports are bad. In terms of recommendations I would go for import artiq.language as aq; ...; @aq.kernel. Along the lines of numpy and similar.

@drewrisinger
Copy link
Contributor Author

@jordens the point is that the compiler can't recognize any sort of prefix, so even with your suggestion, the ARTIQ compiler would throw an error. However, I haven't tested it with that specific combination.

@drewrisinger
Copy link
Contributor Author

Side note: when this is fixed, should change documentation to remove all examples of from artiq.experiment import * because it's bad practice.

@jordens
Copy link
Member

jordens commented Sep 27, 2018

Yes. None of this works right now because of the compiler. My point is not about the compiler but about the recommended style. I didn't want to sidetrack this issue.

@whitequark
Copy link
Contributor

the point is that the compiler can't recognize any sort of prefix, so even with your suggestion, the ARTIQ compiler would throw an error

That can be fixed quite easily, I just forgot about import as when implementing decorator support in the compiler.

@sbourdeauducq
Copy link
Member

The compiler should be fixed, but I don't see much of a problem with the star imports.

@drewrisinger
Copy link
Contributor Author

@sbourdeauducq https://stackoverflow.com/a/2386740
In short, my biggest 2 problems are:

  • hard to manually trace code and figure out where a particular function/class came from
    • Albeit, less of a problem with IDE.
  • Incompatible with linters (i.e. pylint).

Sum total of both of these is that when I'm trying to read other people's code, or write my own code based on others, it feels better to know exactly where other code is coming from instead of just "magic."

@drewrisinger
Copy link
Contributor Author

That can be fixed quite easily, I just forgot about import as when implementing decorator support in the compiler.

@whitequark note that it fails with import artiq.language.core; @artiq.language.core.kernel, as well. So just accounting for import as won't fix it.

@whitequark
Copy link
Contributor

note that it fails with import artiq.language.core; @artiq.language.core.kernel, as well. So just accounting for import as won't fix it.

Same problem. Right now the @kernel annotation is a hack; the kernel environment has a fake decorator there that just does nothing. I think I should just skip decorators entirely when extracting source code from experiments.

@drewrisinger
Copy link
Contributor Author

ping. out of my expertise.

@jbqubit
Copy link
Contributor

jbqubit commented Jan 8, 2019

it feels better to know exactly where other code is coming from instead of just "magic."

I agree with @drewrisinger. Explicit dependencies make ARTIQ Python code easier to understand. It would be nice to support those who don't want to use import *.

@drewrisinger
Copy link
Contributor Author

ping @whitequark

@whitequark
Copy link
Contributor

For now, import kernel explicitly.

@sbourdeauducq
Copy link
Member

@whitequark What is holding this?

@whitequark
Copy link
Contributor

@sbourdeauducq I wasn't sure how to implement this best, but I just came up with a solution to that.

@sbourdeauducq
Copy link
Member

@whitequark Any update?

@pca006132
Copy link
Contributor

A hacky solution, not sure if it works for all circumstances:

--- a/artiq/compiler/transforms/inferencer.py
+++ b/artiq/compiler/transforms/inferencer.py
@@ -6,6 +6,7 @@ from collections import OrderedDict
 from pythonparser import algorithm, diagnostic, ast
 from .. import asttyped, types, builtins
 from .typedtree_printer import TypedtreePrinter
+from artiq.experiment import kernel
 
 
 def is_nested_empty_list(node):
@@ -1662,7 +1663,13 @@ class Inferencer(algorithm.Visitor):
 
     def visit_FunctionDefT(self, node):
         for index, decorator in enumerate(node.decorator_list):
-            if types.is_builtin(decorator.type, "kernel") or \
+            def eval_attr(attr):
+                if isinstance(attr.value, asttyped.QuoteT):
+                    return getattr(attr.value.value, attr.attr)
+                return getattr(eval_attr(attr.value), attr.attr)
+            if isinstance(decorator, asttyped.AttributeT):
+                decorator = eval_attr(decorator)
+            if id(decorator) == id(kernel) or types.is_builtin(decorator.type, "kernel") or \
                     isinstance(decorator, asttyped.CallT) and \
                     types.is_builtin(decorator.func.type, "kernel"):
                 continue

At least works for the following cases:

import artiq.experiment as exp

class Test(exp.EnvExperiment):

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

    @exp.kernel
    def run(self):
        pass

and

import artiq

class Test(artiq.experiment.EnvExperiment):

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

    @artiq.experiment.kernel
    def run(self):
        pass

@drewrisinger
Copy link
Contributor Author

Thanks @pca006132, I honestly got to the point where I forgot about this b/c I was just doing from artiq.language.core import kernel, which feels like a reasonable compromise.

@hartytp
Copy link
Collaborator

hartytp commented Sep 2, 2021

@sbourdeauducq the following experiment run on the current master gives me an error

import artiq.experiment as exp

class Test(exp.EnvExperiment):

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

    @exp.kernel
    def run(self):
        pass
root:While compiling /master/src/desert-gem-experiments/experiments/kernel_test.py
/master/src/desert-gem-experiments/experiments/kernel_test.py:8:5-8:6: error: decorators are not supported
    @exp.kernel
    ^          
``

@sbourdeauducq sbourdeauducq reopened this Sep 2, 2021
@sbourdeauducq
Copy link
Member

Supporting special decorators not simply called as kernel, portable, etc. is difficult to do correctly because of #416. We need to know which modules to cache...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants