Wakoma / nimble

The nimble. An open source, rapidly deployable, wireless mesh network.
CERN Open Hardware Licence Version 2 - Strongly Reciprocal
56 stars 9 forks source link

orchestration script using exsource #14

Closed drayde closed 4 months ago

drayde commented 4 months ago

I had to do this if statement for the module import to allow both exsource/cq-cli and the web server run the code https://github.com/Wakoma/nimble/blob/791da1e2ba7c7905b309d9f430f046b28147e335/doc_generator/mechanical/components/cadquery/rack_leg.py#L4-L7 Do you have a better idea, @jmwright ?

jmwright commented 4 months ago

I think what I've done in some of my other hardware repos is to do import sys and sys.path.append() to manipulate the Python path so that I only ever need one form of the import statement. I think you can put it in an __init__.py so that boiler plate is not required in each model. I don't know if that ends up being better or not though.

drayde commented 4 months ago

I think the path modifications need to be done in the server code in our case, so that it will find non-relative imports. What I am wondering: should there be some cq-cli modification (or option?) to handle this? I guess it's a use case not only for us

jmwright commented 4 months ago

So maybe cq-cli would have an --addpaths option to add to the Python path? It could be formatted similar to how the PTHONPATH variable is done.

cq-cli --infile /path/to/file.py --codec step --addpaths /path/to:/path/to/subdir

Does that seem reasonable?

drayde commented 4 months ago

Turns out cq-cli already does that: https://github.com/CadQuery/cq-cli/blob/9502fafe12069e938eb5b25f8547193e2c2656f1/src/cq_cli/main.py#L91

And modifying the path still does not allow relative imports as this would mean that the script must be treated as a module rather than the main script. Probably not the way we should go, as it will change the cqgi conventions.

So instead, we probably have to change the server code. What do you think about using cqgi there as well instead of loading the code as module?

julianstirling commented 4 months ago

Check out the new version of exsource. It should run cq-cli in a subprocess (without python -m) so it should always be directly run from cq-cli.

jmwright commented 4 months ago

@drayde Does the suggestion by @julianstirling solve the issue?

drayde commented 4 months ago

I'm afraid it doesn't make a difference how you run cq-cli

julianstirling commented 4 months ago

Can you give a short code example so I understand what you mean. The issue is that cq-cli and the web-server must run the same script but they have different conventions for relative imports?

I think it is worth thinking of a non-hacky solution. Even if we have to change how one of them processes files. We want the project to be an example for a customisable methodology. So clean and simple scripts are important.

drayde commented 4 months ago

I agree on finding a non-hacky solution.

To clear up the problem: The cadquery scripts are accessed in 2 ways:

  1. Using cq-cli through exsource. This loads the scripts as main python script and executes them. This way you cannot have relative python imports in the script, as this is only possible if a module context is present
  2. Using import. e.g. https://github.com/Wakoma/nimble/blob/064ceb5a550cd5bdcb5b97fe36718852a0d89301/doc_generator/server/nimble_server.py#L117-L121 This enables relative imports, as the script is loaded as module. The non-relative import fails here, as the imported module is not found in the python search path

We could solve this by:

  1. in nimble_server.py, also use the cq-cli mechanism. We actually do not have to go over the cmd line / run a new process for this, we can also use the underlying cqgi https://cadquery.readthedocs.io/en/latest/cqgi.html
  2. add the imported sub-module non-relatively in both cases and add its path to the python search path in nimble_server.py. Will work, but the cadquery script is accessed in 2 different ways still (calling function vs running main context code), which is not optimal imho
  3. remove access to the scripts in nimble_server.py completely. It is not really needed. We only have it there so individual parts can also be created through the web interface, which is not a requirement, if we create complete packages only. However, it's nice to have it there because a) it might be useful for future use cases b) it serves as an example of how it could be done in other projects

My preferred solution is 1, as to me it looks like the cleanest solution. @jmwright and @julianstirling do you agree?

jmwright commented 4 months ago

I'm ok with doing option 1. @julianstirling ?

julianstirling commented 4 months ago

Option 1 I think is best. It allows us to simply say

"Our CadQuery Scripts are processed with cq-cil"

Simplicity is good when possible

drayde commented 4 months ago

Perfect. I would suggest to merge my changes and open a new issue for this. @jmwright is this something you would like to implement?

jmwright commented 4 months ago

@drayde When you open the issue, please post an example of how the orchestration script should be called now. I'll get the server updated for the latest changes after that.

drayde commented 4 months ago

Thanks for merge @jmwright ! The discussed new issue is here: https://github.com/Wakoma/nimble/issues/15

To clarify: The orchestration script is already called from the web server code. This works fine, nothing to change there. The issue is only about the rack_leg code in nimble_server.py. I guess I don't need to give you example code for using cqgi, you probably know that better than me.