Java/C#: Generalize script for generating flow models. #8506
Conversation
michaelnebel
force-pushed
the
java/generalize-generate-flow-model
branch
6 times, most recently
from
30fef41 to
90f1b75
Compare
There was a problem hiding this comment.
Added some minor comments. It looks plausible, but I haven't yet checked the model generation, so I have no clear view of the process.
I don't know if you know about this, but if you want to test this in an action, you can push it to a repo in the https://github.com/dsp-testing org.
| gitroot = subprocess.check_output(["git", "rev-parse", "--show-toplevel"]).decode("utf-8").strip() | ||
| madpath = os.path.join(gitroot, "misc/scripts/models-as-data/") | ||
| sys.path.append(madpath) |
There was a problem hiding this comment.
I'm not sure what the best way is to import dynamically. Both sys.path.append and importlib.import_module are used in our scripts. (And maybe there are other options too.) I don't know if we have any simpler options, unless we put the scripts next to each other. (I have no preference.) In case of the coverage jobs, we have all the scripts next to each other, and language names, extension types, query locations are stored in config items, and are passed around, see here.
There was a problem hiding this comment.
Alright. That is good to know - then I probably haven't missed anything obvious. I will just give it a bit more thought, whether we should put the language specific parts in the scripts folder (to avoid the uglyness on l 8-10 above).
Thank you very much for the comments!
There was a problem hiding this comment.
We might have to live with the uglyness in the local script. IMO it is nice to have the actual script we execute next to the other stuff, such that it is easier to find.
It is probably easiest, if you get an introduction during this week, if we should talk about some of the other implementation details as well. |
…r python string interpolation).
michaelnebel
force-pushed
the
java/generalize-generate-flow-model
branch
from
c03be2c to
bbe28bc
Compare
|
@tamasvajk : You are probably the best candidate for review'ing. Let me know, if you have any more comments that needs to be incorporated before merging. |
In this PR, we move the generic parts of GenerateFlowModel into a separate class, which can then be used for multiple languages.
At the time of writing the last commit of the PR, then imports the new generic module and instantiates a new object, which is then used for model generation.
My experience with python is limited, but it seems like a bit of hack to modify sys.path before importing the other python module.
@tamasvajk : Do you know a better way of achieving the same?
I suppose one option is to move actual generation into the generate-flow-models.py file and just parameterise the script with "language". If we want a script in the local folder, we can just make a symlink to the shared generator script.