target / theta-idl

Define communication protocols between applications using algebraic data types.
Other
45 stars 9 forks source link

invalid syntax in python modules #43

Closed dmvianna closed 2 years ago

dmvianna commented 2 years ago

If I use theta python -m cat.purr --prefix a -o animals...

Uh-oh.

import animals.cat.purr as cat.purr
SyntaxError: invalid syntax

You can't use namespaces in a module alias in Python.

The problem is, if I have a directory structure such as

types
├── cat
│   ├── purr.theta
│   └── kitty.theta
└── dog
     └── fido.theta

and within fido.theta I have

import cat.purr

This will be impossible to use in Python if the namespaces do not work. I can't just do

export THETA_LOAD_PATH=$HOME/types/dog
theta python -m fido --prefix pipeline -o animals

Because cat is now out of the picture. ☹️

The module ‘ModuleName {namespace = ["cat"], baseName = "purr"}’ was not found in ‘/home/<myname>/types’
istathar commented 2 years ago

Can you run the theta python command from types/ and have it work?

TikhonJelvis commented 2 years ago

Thanks for catching this! Looks like my current tests don't cover it.

The first idea I have for fixing this would be to adapt the Python code generation to use fully qualified names with the prefix everywhere and stop using as altogether. I can take a closer look tomorrow.

Because cat is now out of the picture. ☹️

In this specific example, one workaround would be specifying both directories in THETA_LOAD_PATH:

export THETA_LOAD_PATH=$HOME/types/dog:$HOME/types/cat
theta python -m fido --prefix pipeline -o animals

To make this work, you would also have to change the imports (import purr rather than import cat.purr) since both modules are now at the top level from Theta's point of view. This doesn't seem like a good long-term solution—there is no fundamental reason why having a prefix along with a multilevel namespace should fail—but it illustrates how the load path works and might be useful if the directory structure is imposed by some external constraint rather than being part of the domain that you want to reflect in your schemas.

dmvianna commented 2 years ago

Looking forward to the fix! In the meantime I'll just change the imports by hand. We are very committed to our directory structure and the way the imports work.

dmvianna commented 2 years ago

A better behaviour perhaps would be

import animals.cat.purr as purr

dropping all namespaces in the alias and, in the event of a name clash, returning an error.

To be quite fair, even concatenating the names would be fine:

import animals.cat.purr as cat_purr
TikhonJelvis commented 2 years ago

Yeah, those would both be reasonable approaches. There's still some risk of overlap though, and it turned out to be pretty easy to just use fully qualified references everywhere. It's a bit verbose, but I figure that's a reasonable tradeoff for generated code.

Does #44 actually solve the problem you ran into? I added a couple of modules to test/python that caught the problem, but not sure I covered every case that matters.

dmvianna commented 2 years ago

In the end I found a permutation of the theta command that worked for me:

xport THETA_LOAD_PATH=$HOME/types/dog
theta python -m fido -o animals

I just didn't need to use the --prefix option. But glad I could help finding a corner case for you!