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

Julia preprocessing #311

Merged
merged 18 commits into from
Aug 16, 2022
Merged

Conversation

soumilbaldota
Copy link
Contributor

BEGINRELEASENOTES

  • Adding a julia include (include_jl) in the dictionary passed to the fill_templates

ENDRELEASENOTES

Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @soumilbaldota,

I have a few comments (some are inline) that are easier to handle right at the beginning as they are probably less work to address at the moment. What you have until now definitely goes into the right direction, I just would do some things slightly differently, and some things might not be necessary for the Julia part. However, you are currently working on this on certainly have more insights than me, so if I missed something let me know.

  • The name of the template is currently Julia.jl.jinja2. I think here you have to find a slightly more descriptive name, because there might be more Julia template files and in principle the fact that it is a Julia template is already encoded in the .jl suffix. A good name here should reflect that the template is used for a mutable Julia type/struct.
  • You have put the necessary preprocessing steps for the Julia template into the different existing preprocessing functions for the c++ code. Here I think it would make sense to define an entirely new _preprocess_for_julia (or similar) method, so that all the necessary steps can be found in one place. This makes it much easier to maintain in the future.
  • There are some things that look like they are there simply to reflect the c++ side of code generation. I am not entirely sure they are actually necessary. Making the julia preprocessing a separate method would make it easier to actually spot these and only handle what is actually necessary. (i.e. only add things here if they are needed in the templates)

python/templates/Julia.jl.jinja2 Outdated Show resolved Hide resolved

for relation in datatype['OneToManyRelations'] + datatype['OneToOneRelations']:
if datatype['class'].bare_type != relation.bare_type:
includes_cc.add(self._build_include(relation.bare_type + 'Collection'))
includes_jl.add(self._build_include(relation.bare_type + 'Collection', julia=True))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Respectively can you use the generated files with this? I am asking because as far as I can see there is no XYZCollection.jl that we generate here(?)

@@ -359,29 +375,35 @@ def _preprocess_datatype(self, name, definition):
data = deepcopy(definition)
data['class'] = DataType(name)
data['includes_data'] = self._get_member_includes(definition["Members"])
data['includes_data_jl'] = self._get_member_includes(definition["Members"], julia=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? In the templates I don't see that we use includes_data_jl

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see they are used in the templates now. Is there a reason why they have to go to a different list than the usual includes_jl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they can go in the same list. I will change it in the next commit.


if datatype['VectorMembers']:
includes_cc.add('#include <numeric>')

datatype['includes_coll_cc'] = self._sort_includes(includes_cc)
datatype['includes_coll_data'] = self._sort_includes(includes)
datatype['includes_jl'].update(self._sort_includes(includes_jl))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the _sort_includes work as expected with the Julia includes?

python/templates/Julia.jl.jinja2 Outdated Show resolved Hide resolved
@tmadlener
Copy link
Collaborator

This looks pretty nice already. Is the generated code working? Can you add just a few tests using the example datamodel that comes with podio. I don't think it has to cover everything at this point, but e.g. something using the ExampleCluster, ExampleHit, and ExampleMC would be nice. Maybe we can run them in the CI (although I am not sure if any of our workflow environment actually comes with Julia)

I have one question / comment. Maybe you have a stronger opinion here than me. From the current status it looks like the components use the same template as the datatypes. I don't currently see a technical reason not do so for Julia, since essentially we do not really differentiate between the two (yet?). Do you think it would make sense to introduce a template for components for Julia? This could then obviously be made much simpler than the one for datatypes, since by definition components cannot have any sort of relation or vector members. Or would you rather leave it with one template for the moment and introduce a separate one when it is actually needed?

@soumilbaldota
Copy link
Contributor Author

soumilbaldota commented Jul 6, 2022

@tmadlener

I don't think it has to cover everything at this point, but e.g. something using the ExampleCluster, ExampleHit, and ExampleMC would be nice.

The generated code is working for them and I have run it with datalayout.yaml.

Or would you rather leave it with one template for the moment and introduce a separate one when it is actually needed?

I think I can include this later also.

Currently I am facing an issue that when I run class_generator with edm4hep.yaml. It generates the Vertex.jl and ReconstructedParticle.jl but they are interdependent and try to include each other resulting in a circular inclusion. I am confused on how to fix them.
EDIT: Will need some forward declarations .
The code generated looks fine as per the prototype made earlier.

@tmadlener
Copy link
Collaborator

Currently I am facing an issue that when I run class_generator with edm4hep.yaml. It generates the Vertex.jl and ReconstructedParticle.jl but they are interdependent and try to include each other resulting in a circular inclusion. I am confused on how to fix them.
EDIT: Will need some forward declarations .
The code generated looks fine as per the prototype made earlier.

Ah that is an excellent double check to see if things are working as expected and whether we have overlooked something. Are forward declarations possible in Julia? Or do we have to think about a different solution to make this work with datatypes that refer to each other?

@soumilbaldota
Copy link
Contributor Author

Are forward declarations possible in Julia?

They are not.
If you have a suitable way or idea to fix it please let me know.
And the workarounds I have found are not ideal for our use case (two separate files.)

The below only works when both the structs are in the same file.

mutable struct Vertex{T}
	associatedParticle::Union{T ,Nothing}
end
mutable struct ReconstructedParticle{T}
	startVertex::Union{Nothing, T }
end
function Vertex()
	return Vertex{ReconstructedParticle}(
	nothing,
	)
end
function ReconstructedParticle()
	return ReconstructedParticle{Vertex}(
	nothing,
	)
end

@tmadlener
Copy link
Collaborator

If you have a suitable way or idea to fix it please let me know.

I will have to think about it as well. Maybe there is an elegant solution using abstract types or something, but from the top of my head I am not entirely sure. Maybe @hegner has a clever idea for this.

Another thing that we have not really dealt with in our prototype is the fact that the yaml definition allows to introduce a namespace (in c++), e.g. edm4hep for EDM4hep. Here I think we should be able to use Julia modules to achieve a very similar effect. Could you have a look at that and see what is possible in that regard? One thing that would need clarification from my side is whether we are able to "re-open" a module in different files. That is something that is possible in c++, but for Julia I am not entirely sure.

Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach and implementation for the templates. I have only a few minor comments inline and would also want to ask you whether you could add a couple of tests that use the example datamodel that comes with podio to see if things work with that. At least for the most important use cases like relations.

Could you also have a look at the CI failures and fix any obvious problems. Let me know if you need help with that.

python/podio_class_generator.py Outdated Show resolved Hide resolved
python/templates/Constructor.jl.jinja2 Show resolved Hide resolved
@soumilbaldota
Copy link
Contributor Author

/home/runner/work/podio/podio/tests/src/ExampleWithOneRelationSIOBlock.cc:13:29: error: unused parameter 'device' [-Werror,-Wunused-parameter]
void handlePODDataSIO(devT& device, ::ExampleWithOneRelationData* data, size_t size) {
^
/home/runner/work/podio/podio/tests/src/ExampleWithOneRelationSIOBlock.cc:13:67: error: unused parameter 'data' [-Werror,-Wunused-parameter]
void handlePODDataSIO(devT& device, ::ExampleWithOneRelationData* data, size_t size) {
^
2 errors generated.

The above two errors are making the pre-commit fail but i dont know why.

Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I just realized is that the current version is not working for me, because the generated Julia code is also passed through clang-format, which messes with the formatting and makes the generated Julia code not work in the end.

You will have to add a check here

if self.clang_format:

that makes the Julia files not being run through clang-format. Should be as simple as checking for the file-ending I hope.

@soumilbaldota
Copy link
Contributor Author

I have added Collections in a separate template to Constructor because we are only making Collections of Datatypes and not Components and hence called fill_templates only in process_datatype()

tests/unittest.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. Could you just add one or two tests that also use the collections? Obviously since they are Vector{T} at the moment, there is probably nothing that breaks them, but if we need a more complicated implementation in the future we would at least immediately see where they break.

A question about the template names, resp. the produced filenames: I suppose you have to name it JuliaCollection, because Collection is already taken by the c++ side and a different suffix is not enough to distinguish them, right?

python/templates/Constructor.jl.jinja2 Outdated Show resolved Hide resolved
python/templates/JuliaCollection.jl.jinja2 Outdated Show resolved Hide resolved
python/templates/MutableStruct.jl.jinja2 Outdated Show resolved Hide resolved
soumilbaldota and others added 5 commits August 16, 2022 11:16
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
@tmadlener tmadlener merged commit b0c600e into AIDASoft:julia Aug 16, 2022
tmadlener pushed a commit that referenced this pull request Aug 16, 2022
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
Ananya2003Gupta pushed a commit to Ananya2003Gupta/podio that referenced this pull request Sep 1, 2023
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
Ananya2003Gupta pushed a commit to Ananya2003Gupta/podio that referenced this pull request Sep 26, 2023
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
Ananya2003Gupta pushed a commit to Ananya2003Gupta/podio that referenced this pull request Sep 27, 2023
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
Ananya2003Gupta pushed a commit to Ananya2003Gupta/podio that referenced this pull request Oct 3, 2023
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
Ananya2003Gupta pushed a commit to Ananya2003Gupta/podio that referenced this pull request Nov 14, 2023
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
Ananya2003Gupta pushed a commit to Ananya2003Gupta/podio that referenced this pull request Nov 27, 2023
* adding pre-processing/"include" logic for julia

* Add templates for Julia code
tmadlener added a commit that referenced this pull request Dec 4, 2023
…473)

* Parsing Julia types from c++ types in yaml description (#310)

* Julia preprocessing (#311)

* adding pre-processing/"include" logic for julia

* Add templates for Julia code

* Add Julia unittests to test suite (#322)

* grouping code into modules

* add documentation for Julia code generation (#329)

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

* Added --upstream-edm code generation support

* Created _sort_components_and_datatypes function to perform topological sort

* Added abstract support for builtin Vector Members

* Added conditional check for StaticArrays

* Suggested Changes

* Added ENABLE_JULIA toggle

---------

Co-authored-by: soumilbaldota <71919921+soumilbaldota@users.noreply.github.com>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants