FAIRmat-NFDI / nomad-analysis

This repo contains the standard NOMAD plugin for analysis.
https://fairmat-nfdi.github.io/nomad-analysis/
Apache License 2.0
1 stars 0 forks source link

Testing pipeline #6

Closed JosePizarro3 closed 4 months ago

JosePizarro3 commented 4 months ago

I am testing the pipeline and restructuring a bit the project to look more aligned with the central NOMAD and other plugins which are integrated in the software.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 8285970780

Details


Totals Coverage Status
Change from base Build 8283702209: 0.0%
Covered Lines: 27
Relevant Lines: 32

💛 - Coveralls
JosePizarro3 commented 4 months ago

@ladinesa @ka-sarthak any idea why the pipeline does not work? I just renamed src to analysis and moved the modules one level up (before they were under src/nomad_analysis.

Everything else, I tried to copy from the example, so I am really puzzled.

Btw, as an additional question: do we need to test parsing from nomad.cli? I think we can get rid off these in this repo anyways.

ka-sarthak commented 4 months ago

Hi @JosePizarro3

I had similar test failures earlier and it was probably because the nomad parse was unable to find the analysis schema. If you export the path of the new src (or analysis in this case) to PYTHONPATH, parse function might work. A better solution is to add the name of folder containing the package code to this quantity in pyproject.toml and then re-build the package.

[tool.setuptools.packages.find]
where = ["src"]

But the choice for having src/nomad-analysis was a deliberate one. It's a good practice to have src folder that contains the package code (read more here). I also raised an issue to support this in nomad-schema-plugin-example of which this repo is a fork. https://github.com/nomad-coe/nomad-schema-plugin-example/issues/9

Similar structure is being used for nomad-measurements, nomad-material-properties, and other plugins being developed in Area A.

JosePizarro3 commented 4 months ago

Hi @JosePizarro3

I had similar test failures earlier and it was probably because the nomad parse was unable to find the analysis schema. If you export the path of the new src (or analysis in this case) to PYTHONPATH, parse function might work. A better solution is to add the name of folder containing the package code to this quantity in pyproject.toml and then re-build the package.

[tool.setuptools.packages.find]
where = ["src"]

But the choice for having src/nomad-analysis was a deliberate one. It's a good practice to have src folder that contains the package code (read more here). I also raised an issue to support this in nomad-schema-plugin-example of which this repo is a fork. nomad-coe#9

Similar structure is being used for nomad-measurements, nomad-material-properties, and other plugins being developed in Area A.

Yeah, thanks for the info. My question is why renaming does not work, and the plugin example works with a flat folder structure (and all the other plugins we have ...).

Personally I am not friend of overnesting stuff (we are essentially having 2 folders instead of a flat hierarchy. But I am happy with whatever. We just need to be consistent.

JosePizarro3 commented 4 months ago

One remark on the structure: I think nomad-analysis could be itself a placeholder for individual plugins which then get hooked up; something like:

.
└── nomad-analysis/
    └── src/
        ├── spectral_profiles_analysis/
        │   ├── nomad_plugin.yaml
        │   └── ...
        ├── descriptors_analysis/
        │   ├── nomad_plugin.yaml
        │   └── ...
        └── <whatever>_analysis/
            ├── nomad_plugin.yaml
            └── ...

Still, I am puzzled by the fact that the pipeline mimicking the nomad-parser-plugin-example does not work. I will keep investigating tomorrow.

ka-sarthak commented 4 months ago

Hi @JosePizarro3, I tested the branch in my local and the pytest runs successfully. Even the one using coverage which seems to be failing the action in the workflow.

python -m coverage run -m pytest -sv

I also added some comments on regarding the changes you made. Please review them :)

Regarding the file structure rearrangement, I will still argue in favor of the src-layout, as it makes the testing of the installed package more robust. Also, I think we should have a nomad_analysis module within src to allows for much nicer import statement.

.
└── src/
    └── nomad_analysis/
        ├── spectral_profiles/
        │   ├── nomad_plugin.yaml
        │   └── ...
        ├── descriptors/
        │   ├── nomad_plugin.yaml
        │   └── ...
        └── <whatever>/
            ├── nomad_plugin.yaml
            └── ...

As the installed package will use everything inside /src, the import statements for the installed package would look like:

from nomad_analysis.spectral_profiles import some_class

I think this is more descriptive and navigable than a generic analysis.

aalbino2 commented 4 months ago

@JosePizarro3 don't screw up the plugin structure we have here, we can have a meeting and dicsuss why it is like that before you change it

JosePizarro3 commented 4 months ago

@JosePizarro3 don't screw up the plugin structure we have here, we can have a meeting and dicsuss why it is like that before you change it

Hey, this is just a draft where I didn't want any review yet, nor comments except for my questions.

I wanted to play with the pipeline, because the parser-plugin-example has a very different layout. I want to learn and standarize the way of developing and distributing plugins, so like I said to @ka-sarthak (in private), I am totally ok for using this layout, and adapt this to any other plugin.

aalbino2 commented 4 months ago

@JosePizarro3 don't screw up the plugin structure we have here, we can have a meeting and dicsuss why it is like that before you change it

Hey, this is just a draft where I didn't want any review yet, nor comments except for my questions.

I wanted to play with the pipeline, because the parser-plugin-example has a very different layout. I want to learn and standarize the way of developing and distributing plugins, so like I said to @ka-sarthak (in private), I am totally ok for using this layout, and adapt this to any other plugin.

Oki 😇 Just thought you wanted to change it quick. Anyway, as Sarthak may have explained already, it's mainly for properly pip installability of the plugin, I also didn't enjoy much nesting, but then I found is more convenient when using it

Looking forward to see how this plugin evolves

JosePizarro3 commented 4 months ago

Regarding the file structure rearrangement, I will still argue in favor of the src-layout, as it makes the testing of the installed package more robust. Also, I think we should have a nomad_analysis module within src to allows for much nicer import statement.

.
└── src/
    └── nomad_analysis/
        ├── spectral_profiles/
        │   ├── nomad_plugin.yaml
        │   └── ...
        ├── descriptors/
        │   ├── nomad_plugin.yaml
        │   └── ...
        └── <whatever>/
            ├── nomad_plugin.yaml
            └── ...

As the installed package will use everything inside /src, the import statements for the installed package would look like:

from nomad_analysis.spectral_profiles import some_class

I think this is more descriptive and navigable than a generic analysis.

Yeah, let's keep it src/nomad_analysis. As for now we only have spectra analysis, we keep it without further folder structure, and later we can move modules around.

Ok, I will restore the organization, setup.py, and just change a bit the code. Then it will be ready for review 😛

JosePizarro3 commented 4 months ago

@ka-sarthak this is ready I think.

Please, note the Ruff formatting for your branches in the future 🙂 The pipeline now works as it should regarding ruff format . --check.