FAIRmat-NFDI / nomad-measurements

A NOMAD plugin containing base sections for measurements.
https://FAIRmat-NFDI.github.io/nomad-measurements/
Apache License 2.0
14 stars 1 forks source link

Refactor imports and file structure #84

Closed ka-sarthak closed 9 months ago

ka-sarthak commented 9 months ago

The intention is to have xrd and transmission as sub-packages that can be imported individually without having to import the entire nomad-measurement. For this, the absolute imports in the sub-packages from nomad-measurement needs to be modified to relative imports.

ka-sarthak commented 9 months ago

Some challenges associated to this.

One way to resolve it could be to have utils inside xrd package. But even then, we are also importing NOMADMeasurementsCategory from nomad-measurements, and run into similar issues.

@hampusnasstrom @aalbino2 do you have other ideas?

ka-sarthak commented 9 months ago

I added a commit to the associated branch which moves the absolute import path for nomad_measurements to the init of xrd and transmission packages. With this, the imports to functions and classes of nomad_measurements are based on relative paths.

In other words, it limits the absolute imports for the parent package to the init module. This still requires importing the parent package.

Now, to avoid importing the parent package, if I change these absolute imports to relative ones, I run into ImportError: attempted relative import beyond top-level package. For example, changing from nomad_measurements.utils import * to from ..utils import * in xrd/__init__.py

As per my understanding, unless we move utils and NOMADMeasurementsCategory into each of the measurement sub-packages, we can't avoid importing the parent package.

aalbino2 commented 9 months ago

Hi @ka-sarthak, you may create a "basesection" submodule into nomad-measurements and you place the categories into an init.py there, also the utils may be moved there. I'm doing this for movpe, does it make sense to you? One would still need to import this basesection package, but at least it is not importing all the stuff in nomad measurements at once

ka-sarthak commented 9 months ago

@aalbino2 That might be a good idea. One issue I can see is with the naming of the basesections folder. If we have such a folder for both IKZ and nomad-measurements (and others in future), each of these basesections package should have it's identifier prepended in the name.

Something like:

- schemas/IKZ_basesections:
      python_package: IKZ_basesections
- schemas/nomad_measurements_basesections:
      python_package: nomad_measurements_basesections
aalbino2 commented 9 months ago

It also works without changing the package name:

hampusnasstrom commented 9 months ago

What? How is that possible?

aalbino2 commented 9 months ago

wouldn't it work?

hampusnasstrom commented 9 months ago

No, you can't have two python modules with the same name. But also the import would be:

plugins:
  options:
    schemas/ikz_plugin/basesections:
      python_package: ikz_plugin.basesections
    schemas/nomad_measurements/basesections:
      python_package: nomad_measurements.basesections

no?

aalbino2 commented 9 months ago

ah yeah, it would't work. And in the way you wrote, it would require the load of the entire package, that is what we try to avoid here. So, probably, as @ka-sarthak pointed before can be a solution

hampusnasstrom commented 9 months ago

But anyways, I'm not sure what I think of the name basesections here. All the sections in nomad-measurements (except for ELNXRayDiffraction) are basesections. Is there not a more descriptive submodule name we can use? What is going in there?

hampusnasstrom commented 9 months ago

What do you mean by "load the entire package"? If you mean install it in the docker image, yes. But if you mean load it into the nomad app, then no. It only loads the submodules you specify.

hampusnasstrom commented 9 months ago

I don't think I understand what you want to accomplish here @ka-sarthak. Maybe we can jump in a discord call later and you can explain it to me.

aalbino2 commented 9 months ago

I mean that we want to load the subpackage, e.g. XRD, or movpe, separately, not the nomad-measurement or the IKZ_plugin as a whole. So in this case the path nomad_measurements.basesections or ikz_plugin.basesections is not available. Right?

aalbino2 commented 9 months ago

We can go for "utils" package instead of basesections. It contains utils.py, the categories in the init.py

hampusnasstrom commented 9 months ago

Again, depends on what you mean by "load". If you mean install in the docker image then yes. But it's not imported into python. I.e.

from some_module import submodule

or

import some_module.submodule

will only import the submodule and not anything from the some_module.

aalbino2 commented 9 months ago

I mean load it the nomad.yaml, so I think it's connected with what we pip install?

ka-sarthak commented 9 months ago

Main pain point is that if we have multiple sub-packages under measurements, and if a user is only interested in one of them, they should be able to install only this sub-package (to the nomad yaml and for the docker image). If this sub-package depends on the parent package measurement, they will have to install the whole package, which in-turn installs all the sub-packages.

with @aalbino2 suggestion, we might be able to resolve this by having a general sub-package with required general modules.

src └── nomad_measurements ├── nomad_measurements_basesections │ ├── init.py │ ├── nomad_plugin.yaml │ └── utils.py ├── nomad_measurements_transmission │ ├── init.py │ ├── nomad_plugin.yaml │ ├── parser.py │ └── schema.py ├── nomad_measurements_xrd ├── IKZ.py ├── init.py ├── nomad_plugin.yaml ├── parser │ ├── init.py │ ├── nomad_plugin.yaml │ ├── parser.py ├── readers.py └── schema.py

With something like this, the user doesn't need to pip-install the whole nomad-measurement rather make do with installing xrd and basesections.

aalbino2 commented 9 months ago

yes, so we will review the naming but basically this is the idea

ka-sarthak commented 9 months ago

Given that,