nomad-coe / electronic-parsers

Apache License 2.0
18 stars 7 forks source link

Use run schema plugin #191

Closed ladinesa closed 8 months ago

ladinesa commented 10 months ago

See https://github.com/nomad-coe/nomad-schema-plugin-run.git

ndaelman-hu commented 10 months ago

Will review this afternoon, after my FAIRmat Café.

ndaelman-hu commented 10 months ago

I was merging the CoreHole... I'm going to have to do a massive rebase, won't I?

ndaelman-hu commented 10 months ago

Comparing the output from the updated files for i in $(find . -type f | grep -e '.py$'); do a=$(grep 'import runschema$' $i); if [[ $a ]]; then echo $i; fi; done | sort and the distribution of parser-specific metainfos for i in $(find electronicparsers/*/metainfo/. -type f | grep -e '.py$'); do if [[ ! $(echo $i | grep '__init__.py') ]]; then echo $i; fi; done | sort the following files have not been changed:

electronicparsers/atk/metainfo/./atk.py
electronicparsers/charmm/metainfo/./charmm.py
electronicparsers/fplo/metainfo/./fplo_temporaries.py
electronicparsers/fplo/metainfo/./fplo_input_autogenerated.py
electronicparsers/fplo/metainfo/./fplo.py
electronicparsers/molcas/metainfo/./molcas.py
electronicparsers/openmx/metainfo/./openmx.py

Just double-check them to be sure.

ndaelman-hu commented 9 months ago

There are several unused imports in the metainfo. I guess they can be removed?

searched via for i in $(find . -type f | grep -e '.py$'); do a=$(grep 'simulation$' $i); if [[ $a ]]; then echo $i: $a; fi; done and then manually pruned

./electronicparsers/exciting/metainfo/exciting.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/charmm/metainfo/charmm.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/elk/metainfo/elk.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo_temporaries.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo_input_autogenerated.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/ams/metainfo/ams.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/molcas/metainfo/molcas.py: from nomad.datamodel.metainfo import simulation

Shouldn't simulation.run.Run be replaced too? It's present in many metainfo folders, and some parser.py files.

ladinesa commented 9 months ago

There are several unused imports in the metainfo. I guess they can be removed? searched via for i in $(find . -type f | grep -e '.py$'); do a=$(grep 'simulation$' $i); if [[ $a ]]; then echo $i: $a; fi; done and then manually pruned

./electronicparsers/exciting/metainfo/exciting.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/charmm/metainfo/charmm.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/elk/metainfo/elk.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo_temporaries.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo_input_autogenerated.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/ams/metainfo/ams.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/molcas/metainfo/molcas.py: from nomad.datamodel.metainfo import simulation

Shouldn't simulation.run.Run be replaced too? It's present in many metainfo folders, and some parser.py files.

I do not see any of these simulation.run.Run

JosePizarro3 commented 9 months ago

Hi there,

@JFRudzinski this also interests you.

I am back from my vacation, and I saw this and it draw my attention. I thought that we were going to work out the transition to data and the new schema, in an independent branch in NOMAD, then, we would define a plugin with the new computational schema and migrate the parsers to plugin parsers.

I understand this merge is now creating an intermediate step. We now migrate run into a plugin, which then changes ALL the parsers. Then we define data and once more change ALL the parsers. We can of course do this, but I would avoid doing double-work on having to change all the parsers in a time frame of 2-3 months. Or what is the objective of this migration, @ladinesa?

I understand that for workflow2 this is useful, as it is anyways very up-to-date, but for run I think is overkill.

ladinesa commented 9 months ago

Hi there,

@JFRudzinski this also interests you.

I am back from my vacation, and I saw this and it draw my attention. I thought that we were going to work out the transition to data and the new schema, in an independent branch in NOMAD, then, we would define a plugin with the new computational schema and migrate the parsers to plugin parsers.

I understand this merge is now creating an intermediate step. We now migrate run into a plugin, which then changes ALL the parsers. Then we define data and once more change ALL the parsers. We can of course do this, but I would avoid doing double-work on having to change all the parsers in a time frame of 2-3 months. Or what is the objective of this migration, @ladinesa?

I understand that for workflow2 this is useful, as it is anyways very up-to-date, but for run I think is overkill.

This addresses Area D's effort to transition into separating domain-specific parts of the nomad lab package from the core infrastructure. This can be done independently of the transition into data. The idea is to have a core nomad lab package that is used by the plug ins. The new schemas and parsers for when we do the transition into data will all be plug ins and we can put it them anywhere you like, not necessarily on the same plug that are defined in this pr.

JosePizarro3 commented 9 months ago

This addresses Area D's effort to transition into separating domain-specific parts of the nomad lab package from the core infrastructure. This can be done independently of the transition into data. The new schemas and parsers for when we do the transition into data will all be plug ins and you can put it them anywhere you like, not necessarily on the same plug that are defined in this pr.

Sure, I understand. Just mentioning that the imports are going to change anyways in 2-3 months, and that the nomad-schema-plugin-run git will be soon deprecated.

ladinesa commented 9 months ago

This addresses Area D's effort to transition into separating domain-specific parts of the nomad lab package from the core infrastructure. This can be done independently of the transition into data. The new schemas and parsers for when we do the transition into data will all be plug ins and you can put it them anywhere you like, not necessarily on the same plug that are defined in this pr.

Sure, I understand. Just mentioning that the imports are going to change anyways in 2-3 months, and that the nomad-schema-plugin-run git will be soon deprecated.

Yes of course, but we also would like to work on this nomad core package now. Actually this would make the transition to data seamless since you will then not have to worry about any references to the old schema in the core nomad.

ndaelman-hu commented 9 months ago

There are several unused imports in the metainfo. I guess they can be removed? searched via for i in $(find . -type f | grep -e '.py$'); do a=$(grep 'simulation$' $i); if [[ $a ]]; then echo $i: $a; fi; done and then manually pruned

./electronicparsers/exciting/metainfo/exciting.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/charmm/metainfo/charmm.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/elk/metainfo/elk.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo_temporaries.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo_input_autogenerated.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/fplo/metainfo/fplo.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/ams/metainfo/ams.py: from nomad.datamodel.metainfo import simulation
./electronicparsers/molcas/metainfo/molcas.py: from nomad.datamodel.metainfo import simulation

Shouldn't simulation.run.Run be replaced too? It's present in many metainfo folders, and some parser.py files.

I do not see any of these simulation.run.Run

Ha, sorry, probably was on the wrong branch. Yeah, it seems taken care off.

ndaelman-hu commented 9 months ago

I was merging the CoreHole... I'm going to have to do a massive rebase, won't I?

@ladinesa For outstanding PRs in electronicparsers, do we now have to migrate to this new repo? How should we transition fluently? If migrating is quite involved, could we possible first merge (I have only 1 PR) by a certain date, e.g. 20.12?

ladinesa commented 9 months ago

I was merging the CoreHole... I'm going to have to do a massive rebase, won't I?

@ladinesa For outstanding PRs in electronicparsers, do we now have to migrate to this new repo? How should we transition fluently? If migrating is quite involved, could we possible first merge (I have only 1 PR) by a certain date, e.g. 20.12?

The plan is to have each code its own parser plug in. So once chema is done with the new schena for data, I will slowly move each electronicparser to their own plug in, I will see if there is a PR that concerns this particular parser, if there is, I will probably ask you to finalize it or I will skip it then come back once you are done.

ndaelman-hu commented 9 months ago

I was merging the CoreHole... I'm going to have to do a massive rebase, won't I?

@ladinesa For outstanding PRs in electronicparsers, do we now have to migrate to this new repo? How should we transition fluently? If migrating is quite involved, could we possible first merge (I have only 1 PR) by a certain date, e.g. 20.12?

The plan is to have each code its own parser plug in. So once chema is done with the new schena for data, I will slowly move each electronicparser to their own plug in, I will see if there is a PR that concerns this particular parser, if there is, I will probably ask you to finalize it or I will skip it then come back once you are done.

Okay, so in the short-term, we can keep working as is, great! Pls note that as the schema changes, you may also have to update the new run repo. Let us know if we can help out there.

I'm fully on-board with the individual parser plug-ins. Thank you for taking care of this migration!

ladinesa commented 9 months ago

changes

I suggest we continue all schema devs in the new simulation data schema. Yes, we can handle this migration, no worries.

JosePizarro3 commented 8 months ago

This branch has not been merged in develop, but Gitlab is pointed to it.

ladinesa commented 8 months ago

I will simply merge this.