Incorporating Julia code generation in the existing Python interface #473

Ananya2003Gupta · 2023-09-01T12:42:07Z

BEGINRELEASENOTES

Added Julia code generation support in the existing Python interface.
Implemented a new design structure for generated Julia code.
Added default parameters in constructor definitions with support for Abstract types (for builtins).
Created _sort_components_and_datatypes function to perform topological sort on components and datatypes.
Created _has_static_arrays_import to check for the need of using Static Arrays in the generated julia code.
Added --lang (-l) programming language argument to specify the programming language for code generation, current choices: cpp and julia, default: cpp.
Added --upstream-edm code generation support for julia.
Added tests in the unit test suite, covering the Julia code generation of the example data models.
Added documentation for julia code generation.
Added ENABLE_JULIA toggle option. By default it is OFF.

ENDRELEASENOTES

graeme-a-stewart

Hi - looks pretty good, a few comments embedded. Are the tests as extensive as the tests for the C++ rendering of the data model? It's not too exhaustive.

Also, we need to rethink the "forward declaration" solution we use too.

doc/templates.md

python/podio/generator_utils.py

graeme-a-stewart · 2023-09-29T07:57:07Z

python/podio/generator_utils.py

+  builtin_types_map = {"int": "Int32", "float": "Float32", "double": "Float64",
+                       "bool": "Bool", "long": "Int32", "unsigned int": "UInt32",
+                       "unsigned long": "UInt32", "char": "Char", "short": "Int16",
+                       "long long": "Int64", "unsigned long long": "UInt64"}


Maybe long long should be Int128? I am not sure though, as this would be a slow datatype in practice.

long long is 64 bits in practice at the moment (as far as the table on the integers shows). We could put in a header that encodes these "assumptions" into the c++ code, that essentially consists of static_asserts of the form

static_assert(sizeof(long long int) == 8, "long long int is not 64 bits wide");

graeme-a-stewart · 2023-09-29T09:09:03Z

BTW, you also failed some pylint tests that should be fixed

Ananya2003Gupta · 2023-10-03T16:58:02Z

I have corrected all the changes that were asked.

jmcarcell · 2023-10-03T18:27:58Z

Is this optional? I would say even if Julia is installed there will be cases when we'll want podio not to have the Julia interface by default. Of course if Julia is not installed then it should work as it works now, tests included

Ananya2003Gupta · 2023-10-03T19:31:28Z

Is this optional? I would say even if Julia is installed there will be cases when we'll want podio not to have the Julia interface by default. Of course if Julia is not installed then it should work as it works now, tests included

Yes, the code generation will work regardless of whether julia is present in the system or not, as julia code for a particular datamodel is generated using jinja2 templates. And yes the tests for julia will only run if the users have julia in their system.

Ananya2003Gupta · 2023-10-03T19:33:29Z

Is this optional? I would say even if Julia is installed there will be cases when we'll want podio not to have the Julia interface by default. Of course if Julia is not installed then it should work as it works now, tests included

Also, that's why we have introduced a new parameter --lang (-l). By default the language is cpp, but if the users want to generate julia code, they can by specifying -l=julia or --lang=julia.

graeme-a-stewart

Hi - I think this isn't the last word on the EDM in Julia, but it works and allows us to develop from here without very painful rebases, so I am happy to merge this now.

hegner · 2023-11-13T13:04:31Z

@Ananya2003Gupta @graeme-a-stewart - I'd agree once the conflicts are resolved ;-)

Ananya2003Gupta · 2023-11-14T10:34:38Z

@Ananya2003Gupta @graeme-a-stewart - I'd agree once the conflicts are resolved ;-)

Yes, it's done : )

graeme-a-stewart · 2023-11-14T13:26:57Z

Hi @Ananya2003Gupta

I got a test failure when I was running in your branch:

ERROR: LoadError: ArgumentError: Package StaticArrays not found in current path.
- Run `import Pkg; Pkg.add("StaticArrays")` to install the StaticArrays package.

Can you fix that one? You shouldn't make assumptions about which packages are installed in the test environment.

tmadlener · 2023-11-14T15:04:05Z

I think this would just need to be put into the README as a dependency and also how to install it, right? CI seems to be configured to do that and the tests are running and succeeding.

Ananya2003Gupta · 2023-11-14T15:41:38Z

Hi @Ananya2003Gupta

I got a test failure when I was running in your branch:
ERROR: LoadError: ArgumentError: Package StaticArrays not found in current path.
- Run `import Pkg; Pkg.add("StaticArrays")` to install the StaticArrays package.
Can you fix that one? You shouldn't make assumptions about which packages are installed in the test environment.

Actually you would have to install StaticArrays package in your system, then only it would work.
This can be done: https://discourse.julialang.org/t/install-a-packages-from-a-list/30920/6

Ananya2003Gupta · 2023-11-14T15:43:42Z

I think this would just need to be put into the README as a dependency and also how to install it, right? CI seems to be configured to do that and the tests are running and succeeding.

Yes. The users need to have StaticArrays package installed in their system. Otherwise it will throw this error and ask them to install it. We can make it a dependency as suggested here: https://discourse.julialang.org/t/install-a-packages-from-a-list/30920/6

graeme-a-stewart · 2023-11-14T17:48:11Z

Actually you would have to install StaticArrays package in your system, then only it would work. This can be done: https://discourse.julialang.org/t/install-a-packages-from-a-list/30920/6

But you should not fail the test, we are testing the code, not the user's environment. Probably the best way is to have a Project.toml specifically for the test.

For the user, document that StaticArrays is required, but the user should not have to install it in their default Julia environment (which is really bad practice).

Ananya2003Gupta · 2023-11-15T07:27:19Z

Actually you would have to install StaticArrays package in your system, then only it would work. This can be done: https://discourse.julialang.org/t/install-a-packages-from-a-list/30920/6

But you should not fail the test, we are testing the code, not the user's environment. Probably the best way is to have a Project.toml specifically for the test.

For the user, document that StaticArrays is required, but the user should not have to install it in their default Julia environment (which is really bad practice).

Okay

Ananya2003Gupta · 2023-11-18T13:53:40Z

I have added a try-catch block in unittest.jl to check if StaticArrays is available, if not, install it locally.

Ananya2003Gupta · 2023-11-24T10:53:18Z

@hegner @tmadlener can you please review the PR? If there are anymore changes required I would do them.

tmadlener

I can't comment too much on the generated Julia code. @graeme-a-stewart and @hegner are better equipped to comment on that. There are a few minor things (see also the inline comments)

I would merge params.jinja2 and abstracttypes.jinja2 into one julia_helpers.jinja2 file and include that where necessary in the main jinja2 template files
The unittest.jl file should be in tests/unittests, rather than in tests
I would like to have an option ENABLE_JULIA in the top level CMakeLists.txt to toggle Julia support manually, rather than relying on finding the Julia executable alone.
Can you bring back the BEGINRELEASENOTES and ENDRELEASENOTES in the top level comment of the PR? Those are used as delimiter for our tagging script.

More generally, I am not sure whether mixing in the Julia code generator into the existing c++ code generator is the best idea, as that is already complicated enough on its own and sprinkling a few if lang == "xxx" in there doesn't help in that regard. Having said that, I think we can also fix this after this PR and ideally split them into to separate generator classes that each handle exactly one language.

cmake/podioMacros.cmake

python/podio_gen/generator_utils.py

python/podio_class_generator.py

…ude in CI

…l sort

tmadlener · 2023-11-27T10:34:21Z

The basic idea for splitting classes would be to have a sort of base class that does the common processing and them two dedicated classes for cpp and julia respectively that deal with the specifics. However, as I said, I would first get this PR merged as it is with the things mixed and then we can separate things later, when there are no multiple other PRs touching code generation in flight, so that we don't have to keep all of them synchronized.

Ananya2003Gupta · 2023-11-27T10:39:13Z

The basic idea for splitting classes would be to have a sort of base class that does the common processing and them two dedicated classes for cpp and julia respectively that deal with the specifics. However, as I said, I would first get this PR merged as it is with the things mixed and then we can separate things later, when there are no multiple other PRs touching code generation in flight, so that we don't have to keep all of them synchronized.

Got it.
Also, I am not sure why the key4hep / build-and-test (sw-nightlies.hsf.org/key4hep, ON) (pull_request) is failing.

 [374/381] Linking CXX shared library tests/schema_evolution/libTestDataModel_v3SioBlocks.so
  [375/381] Generating TestDataModel_v3Dict.cxx, TestDataModel_v3DictDict.rootmap
  Warning: Unused class rule: ExampleWithUserInitv2
  Warning: Unused class rule: ex42::ExampleWithNamespacev2
  Warning: Unused class rule: ExampleWithArrayv2
  [376/381] Building CXX object tests/schema_evolution/root_io/CMakeFiles/write_old_data_root.dir/write_old_data_root.cpp.o
  [377/381] Linking CXX executable tests/schema_evolution/root_io/write_old_data_root
  [378/381] Building CXX object tests/schema_evolution/root_io/CMakeFiles/read_new_data_root.dir/read_new_data_root.cpp.o
  [379/381] Building CXX object tests/schema_evolution/CMakeFiles/TestDataModel_v3Dict.dir/TestDataModel_v3Dict.cxx.o
  [380/381] Linking CXX shared library tests/schema_evolution/libTestDataModel_v3Dict.so
  [381/381] Linking CXX executable tests/schema_evolution/root_io/read_new_data_root
  ninja: build stopped: cannot make progress due to previous errors.
  Error: Process completed with exit code 1.

tmadlener · 2023-11-27T10:44:35Z

That looks like a temporary issue unrelated to your changes here: https://github.com/AIDASoft/podio/actions/runs/7003341864/job/19048875848?pr=473#step:4:986

We will simply use the next run if there are more changes, or rerun this later and it should pass.

Ananya2003Gupta · 2023-11-27T10:47:27Z

I would like to have an option ENABLE_JULIA in the top level CMakeLists.txt to toggle Julia support manually, rather than relying on finding the Julia executable alone.

For this, should I keep an ENABLE_JULIA toggle like ENABLE_SIO ? If julia executable is found, enable the toggle to on and then run the tests written for julia in tests/CMakeLists.txt?

Ananya2003Gupta · 2023-11-27T10:49:00Z

That looks like a temporary issue unrelated to your changes here: https://github.com/AIDASoft/podio/actions/runs/7003341864/job/19048875848?pr=473#step:4:986

We will simply use the next run if there are more changes, or rerun this later and it should pass.

Okay. Yes I need to make one more change for ENABLE_JULIA. I may do the rebase as well. And force push the changes.

tmadlener · 2023-11-27T10:53:36Z

For this, should I keep an ENABLE_JULIA toggle like ENABLE_SIO ? If julia executable is found, enable the toggle to on and then run the tests written for julia in tests/CMakeLists.txt?

Yes, I think doing the same thing as ENABLE_SIO is a good idea, i.e. if the users wants Julia, but doesn't have Julia we can simply fail at cmake stage.

Ananya2003Gupta · 2023-11-27T10:55:13Z

For this, should I keep an ENABLE_JULIA toggle like ENABLE_SIO ? If julia executable is found, enable the toggle to on and then run the tests written for julia in tests/CMakeLists.txt?

Yes, I think doing the same thing as ENABLE_SIO is a good idea, i.e. if the users wants Julia, but doesn't have Julia we can simply fail at cmake stage.

Okay. So, the datamodels for julia would only be generated when ENABLE_JULIA is ON and so the julia tests will run?

tmadlener · 2023-11-27T12:13:08Z

Yes, exactly.

Ananya2003Gupta · 2023-11-27T14:03:21Z

I have added the ENABLE_JULIA toggle button.
We can re run the two failed tests.

Ananya2003Gupta · 2023-11-28T07:43:15Z

@tmadlener I have made all the asked changes.

tmadlener

Very nice. Thank you. I have just a few final minor comments. I can also just do them via the github web interface.

I will let @hegner or @graeme-a-stewart press the button.

tests/CMakeLists.txt

.github/workflows/key4hep.yml

Ananya2003Gupta changed the title ~~Juliareset~~ [WIP] Julia PODIO Interface Sep 1, 2023

Ananya2003Gupta changed the title ~~[WIP] Julia PODIO Interface~~ [WIP] Incorporating Julia code generation in the existing Python interface Sep 13, 2023

Ananya2003Gupta force-pushed the juliareset branch 3 times, most recently from 7a4eafe to 3d970f0 Compare September 27, 2023 14:09

graeme-a-stewart suggested changes Sep 29, 2023

View reviewed changes

Ananya2003Gupta force-pushed the juliareset branch from 3d970f0 to cd44b51 Compare October 3, 2023 16:30

Ananya2003Gupta changed the title ~~[WIP] Incorporating Julia code generation in the existing Python interface~~ Incorporating Julia code generation in the existing Python interface Oct 3, 2023

Ananya2003Gupta requested review from tmadlener and graeme-a-stewart October 3, 2023 19:52

Ananya2003Gupta changed the title ~~Incorporating Julia code generation in the existing Python interface~~ [WIP] Incorporating Julia code generation in the existing Python interface Oct 13, 2023

graeme-a-stewart approved these changes Nov 13, 2023

View reviewed changes

Ananya2003Gupta force-pushed the juliareset branch from cd44b51 to 96bdc65 Compare November 14, 2023 10:32

Ananya2003Gupta changed the title ~~[WIP] Incorporating Julia code generation in the existing Python interface~~ Incorporating Julia code generation in the existing Python interface Nov 14, 2023

tmadlener reviewed Nov 24, 2023

View reviewed changes

Ananya2003Gupta added 11 commits November 27, 2023 15:31

Changes related to precommit

64e9ba4

Made changes to correct failing tests

07467e2

Sorted includes and parametric types, added StaticArrays package incl…

4e177b8

…ude in CI

Corrected the Position of StaticArrays Package Install in CI

d2c8a82

Added abstract types in default parameters and '-l' language argument

73362ff

Added --upstream-edm code generation support

ed13342

Fixed pylint issue and documentation + Added tests

8d806f0

Created _sort_components_and_datatypes function to perform topologica…

6ba6715

…l sort

Added abstract support for builtin Vector Members

0db2e39

Added conditional check for StaticArrays

e55926e

Suggested Changes

6e19bee

Added ENABLE_JULIA toggle

5b62bb9

Ananya2003Gupta force-pushed the juliareset branch from 6cdc07e to 5b62bb9 Compare November 27, 2023 13:48

made cpp_type argument required in get_julia_type

37196d4

tmadlener changed the title ~~[WIP] Incorporating Julia code generation in the existing Python interface~~ Incorporating Julia code generation in the existing Python interface Nov 28, 2023

tmadlener approved these changes Nov 28, 2023

View reviewed changes

tests/CMakeLists.txt Outdated Show resolved Hide resolved

tests/CMakeLists.txt Outdated Show resolved Hide resolved

.github/workflows/key4hep.yml Outdated Show resolved Hide resolved

Made suggested changes for julia tests

2253981

tmadlener merged commit 360dad2 into AIDASoft:master Dec 4, 2023
17 checks passed

tmadlener mentioned this pull request Dec 7, 2023

Refactor the code generation and split it into language specific generators #527

Merged

Ananya2003Gupta referenced this pull request Dec 15, 2023

Release Notes for v00-17-04

bf5876f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incorporating Julia code generation in the existing Python interface #473

Incorporating Julia code generation in the existing Python interface #473

Ananya2003Gupta commented Sep 1, 2023 •

edited

Loading

graeme-a-stewart left a comment

graeme-a-stewart Sep 29, 2023

tmadlener Sep 29, 2023

graeme-a-stewart commented Sep 29, 2023

Ananya2003Gupta commented Oct 3, 2023

jmcarcell commented Oct 3, 2023

Ananya2003Gupta commented Oct 3, 2023

Ananya2003Gupta commented Oct 3, 2023 •

edited

Loading

graeme-a-stewart left a comment

hegner commented Nov 13, 2023

Ananya2003Gupta commented Nov 14, 2023

graeme-a-stewart commented Nov 14, 2023

tmadlener commented Nov 14, 2023

Ananya2003Gupta commented Nov 14, 2023 •

edited

Loading

Ananya2003Gupta commented Nov 14, 2023 •

edited

Loading

graeme-a-stewart commented Nov 14, 2023

Ananya2003Gupta commented Nov 15, 2023

Ananya2003Gupta commented Nov 18, 2023

Ananya2003Gupta commented Nov 24, 2023

tmadlener left a comment

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

Ananya2003Gupta commented Nov 28, 2023

tmadlener left a comment

Incorporating Julia code generation in the existing Python interface #473

Incorporating Julia code generation in the existing Python interface #473

Conversation

Ananya2003Gupta commented Sep 1, 2023 • edited Loading

graeme-a-stewart left a comment

Choose a reason for hiding this comment

graeme-a-stewart Sep 29, 2023

Choose a reason for hiding this comment

tmadlener Sep 29, 2023

Choose a reason for hiding this comment

graeme-a-stewart commented Sep 29, 2023

Ananya2003Gupta commented Oct 3, 2023

jmcarcell commented Oct 3, 2023

Ananya2003Gupta commented Oct 3, 2023

Ananya2003Gupta commented Oct 3, 2023 • edited Loading

graeme-a-stewart left a comment

Choose a reason for hiding this comment

hegner commented Nov 13, 2023

Ananya2003Gupta commented Nov 14, 2023

graeme-a-stewart commented Nov 14, 2023

tmadlener commented Nov 14, 2023

Ananya2003Gupta commented Nov 14, 2023 • edited Loading

Ananya2003Gupta commented Nov 14, 2023 • edited Loading

graeme-a-stewart commented Nov 14, 2023

Ananya2003Gupta commented Nov 15, 2023

Ananya2003Gupta commented Nov 18, 2023

Ananya2003Gupta commented Nov 24, 2023

tmadlener left a comment

Choose a reason for hiding this comment

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

tmadlener commented Nov 27, 2023

Ananya2003Gupta commented Nov 27, 2023

Ananya2003Gupta commented Nov 28, 2023

tmadlener left a comment

Choose a reason for hiding this comment

Ananya2003Gupta commented Sep 1, 2023 •

edited

Loading

Ananya2003Gupta commented Oct 3, 2023 •

edited

Loading

Ananya2003Gupta commented Nov 14, 2023 •

edited

Loading

Ananya2003Gupta commented Nov 14, 2023 •

edited

Loading