Removed nodes from sisl

[pfebrer]

The functionality in sisl.nodes has been detached into a new package: nodify (https://pypi.org/project/nodify/).

nodify is now an optional dependency required for the viz module to work.

The nodify package is a pure python package with no dependencies, so it shouldn't give any problems.

However, it needs a minimum python version of 3.9, which will be the minimum version for sisl in may, so it also shouldn't be a problem :)

[pfebrer]

I don't understand why CI fails, it fails even before installing python 😅

[zerothi]

It could be #755, lets wait a day or so... Otherwise I will probably revert it...

[zerothi]

Actually, it couldn't since that is a different work-flow... Sorry for the blurp!

[pfebrer]

It fails when it is installing the compilers. Maybe https://github.com/fortran-lang/setup-fortran should be used instead of directly calling apt-get

[zerothi]

Now the problem is there because nodify isn't a package (it can't pip install it?)

[zerothi]

Annoyingly I can't get it to run on 3.9 since 3.8 failed. :(

Perhaps we really should just bump to 3.9!

Lets do that, a PR that fixes 3.9 problems and deprecates 3.8.

[pfebrer]

Yes, It is a package, but can only be installed on >=3.9.

We can just wait until 3.8 is dropped according to your schedule, no rush :)

[zerothi]

rebase, and lets try again!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.25%. Comparing base (8d6da59) to head (2831533).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
+ Coverage   86.91%   87.25%   +0.34%     
==========================================
  Files         410      393      -17     
  Lines       51826    50145    -1681     
==========================================
- Hits        45042    43754    -1288     
+ Misses       6784     6391     -393     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zerothi]

Sorry for another go here. Could you rebase, I am trying to add another test that checks without viz. :)

[zerothi]

The tests shows it should be more hidden, i.e. Since it is optional there needs to be a fallback on it.

[pfebrer]

Hmm but shouldn't the tests that don't want to test sisl.viz simply not import sisl.viz?

If you want to import sisl.viz then there are extra dependencies which are mandatory, not optional.

[pfebrer]

I mean the error comes from the fact that to define the viz tests you need to import sisl.viz. But isn't there a way to tell pytest to ignore everything inside sisl/viz?

[zerothi]

I mean the error comes from the fact that to define the viz tests you need to import sisl.viz. But isn't there a way to tell pytest to ignore everything inside sisl/viz?

Well, the tests are meant for end-users to also test their installation, and they won't know the difference. Hence, the tests should be self-contained through importer-skips, and if importing fails, I think we should be able to notify the users more succintly.
If they simply do import sisl.viz, and get these weird errors, it ain't helping them. So I think it would be wisests to fix the tests so they work regardless, and as you can see in https://github.com/zerothi/sisl/actions/runs/8849176543, the culprit is actually this PR, since it tries to import things which breaks the import cycle.

[pfebrer]

I think a better solution would be to have a TEST_VIZ environment variable or something like that, which should make pytest not gather tests in sisl/viz.

Otherwise you are asking that import sisl.viz should work regardless of whether the module will be functional or not. I think this doesn't make any sense, because it will result in even weirder errors down the line. An error like "Could not find package X" is very well known by users and their first reaction will be to pip install X, which will work.

I think that requiring import sisl.viz to be importable when its dependencies are not there is not how you want the package to work. It is a requirement that you want to impose because of the way pytest works (i.e. you want pytest to show that these tests exist but are skipped). I think pytest shouldn't determine the way the package works. So instead I would propose that whatever hack that is needed for pytest to show the logs that you want (e.g. mocking sisl.viz) is done in the test phase.

Another thing that I don't understand is why do you want the tests not to fail if there wasn't any indication that the user didn't want those tests. A user might run the tests and see that there are no errors but then run some plot and see that there are indeed errors. This I would argue would be much more confusing than if they failed because of a missing import.

[zerothi]

I think a better solution would be to have a TEST_VIZ environment variable or something like that, which should make pytest not gather tests in sisl/viz.
No, I don't think that should be done, env-vars shouldn't control this.

It needs to be done so it also works in user environments.

Otherwise you are asking that import sisl.viz should work regardless of whether the module will be functional or not. I think this doesn't make any sense, because it will result in even weirder errors down the line. An error like "Could not find package X" is very well known by users and their first reaction will be to pip install X, which will work.

No, I am not, I am suggesting that sisl.viz will report if it doesn't work (raise ImportError) so end users can figure this out, and that the tests automatically discovers this.

I think that requiring import sisl.viz to be importable when its dependencies are not there is not how you want the package to work. It is a requirement that you want to impose because of the way pytest works (i.e. you want pytest to show that these tests exist but are skipped). I think pytest shouldn't determine the way the package works. So instead I would propose that whatever hack that is needed for pytest to show the logs that you want (e.g. mocking sisl.viz) is done in the test phase.

I think you misunderstood me here.

Another thing that I don't understand is why do you want the tests not to fail if there wasn't any indication that the user didn't want those tests. A user might run the tests and see that there are no errors but then run some plot and see that there are indeed errors. This I would argue would be much more confusing than if they failed because of a missing import.

I don't want the tests to not fail, I want the tests to skip! Just like we have importorskips. And currently these work well for everything, but this PR messes that up. ;)

[pfebrer]

No, I am not, I am suggesting that sisl.viz will report if it doesn't work (raise ImportError) so end users can figure this out, and that the tests automatically discovers this.

Ah ok, but it already raises a ModuleNotFoundError. In fact if you look at the logs you will see it on the first error. Then the second time around it fails because of the already present env variable, but that's something that sisl does, I don't know why. I have seen it happen other times, when you try to import after a failed import, you get this.

I don't want the tests to not fail, I want the tests to skip! Just like we have importorskips. And currently these work well for everything, but this PR messes that up. ;)

But how do you know if the user wants them to skip or they just forgot to install the dependencies, if there is no flag for it? 😅

[pfebrer]

Ok, so I understand that every time import sisl.viz is attempted sisl.viz manages to get to the part where env variables are registered, which is before trying to import nodify, which is where it fails the first time.

Still, I don't see how the importorskip would work, because to even define the test functions you need to import sisl.viz.

[zerothi]

No, I am not, I am suggesting that sisl.viz will report if it doesn't work (raise ImportError) so end users can figure this out, and that the tests automatically discovers this.

Ah ok, but it already raises a ModuleNotFoundError. In fact if you look at the logs you will see it on the first error. Then the second time around it fails because of the already present env variable, but that's something that sisl does, I don't know why. I have seen it happen other times, when you try to import after a failed import, you get this.

Yes, I believe it has to do with the way pytest stores things in its runs...

I don't want the tests to not fail, I want the tests to skip! Just like we have importorskips. And currently these work well for everything, but this PR messes that up. ;)

But how do you know if the user wants them to skip or they just forgot to install the dependencies, if there is no flag for it? 😅

If they want to skip them, they should do pytest ... -k "not viz" done. That should be part of the documentation, I don't think this qualifies for a special handling. ;)

Ok, so I understand that every time import sisl.viz is attempted sisl.viz manages to get to the part where env variables are registered, which is before trying to import nodify, which is where it fails the first time.

Still, I don't see how the importorskip would work, because to even define the test functions you need to import sisl.viz.

Hmm, but see my link to the previous CI, it ran a test without viz, but still managed to skip the tests, without breaking.
You have all dependencies wrapped in viz.figure.__init__, so it never breaks because all dependencies are first checked when needed. However, nodify isn't. It is just happily imported whenever. Perhaps the figure code should warn in case there are no backends available.

[pfebrer]

If they want to skip them, they should do pytest ... -k "not viz" done. That should be part of the documentation, I don't think this qualifies for a special handling. ;)

Yes, that looks fine. But I don't see how it can be easily acheived in a way that makes sense.

Hmm, but see my link to the previous CI, it ran a test without viz, but still managed to skip the tests, without breaking.
You have all dependencies wrapped in viz.figure.init, so it never breaks because all dependencies are first checked when needed. However, nodify isn't. It is just happily imported whenever. Perhaps the figure code should warn in case there are no backends available.

But there is a difference. The backends are optional, because the viz module should be able to run only with the backend of your choice. However nodify is a dependency that is mandatory for sisl.viz to work. It is happily imported just like numpy or scipy are happily imported in sisl, because they are mandatory dependencies.

I understand what you want to acheive, but I don't think modifying the logic inside the package just to fit the way pytest works makes much sense.

Isn't there a workaround that you can think of to apply on sisl.viz.conftest? Something like trying to import sisl.viz and if it is not possible mocking it so that the test functions can be defined ?

[zerothi]

Ok, so the above seems to work. I still think we could provide a better error message in case they don't have nodiy and friends installed, but lets see.

[pfebrer]

For me the problem is how to determine if they want to test without sisl.viz or they just forgot to add the dependencies.

What you said pytest -k "not viz" seems great to me, so maybe if we can detect that when we check the imports we can give helpful information to the user.

[zerothi]

I am now not discussing tests, I am thinking of a user who does:

pip install sisl

then does:

import sisl.viz

they will face:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nicpa/codes/sisl/src/sisl/viz/__init__.py", line 34, in <module>
    from . import _xarray_accessor
  File "/home/nicpa/codes/sisl/src/sisl/viz/_xarray_accessor.py", line 17, in <module>
    from .processors.orbital import reduce_orbital_data, split_orbitals
  File "/home/nicpa/codes/sisl/src/sisl/viz/processors/orbital.py", line 23, in <module>
    from ..data import Data
  File "/home/nicpa/codes/sisl/src/sisl/viz/data/__init__.py", line 6, in <module>
    from .bands import BandsData
  File "/home/nicpa/codes/sisl/src/sisl/viz/data/bands.py", line 22, in <module>
    from ..data_sources import FileDataSIESTA, HamiltonianDataSource
  File "/home/nicpa/codes/sisl/src/sisl/viz/data_sources/__init__.py", line 6, in <module>
    from .atom_data import *
  File "/home/nicpa/codes/sisl/src/sisl/viz/data_sources/atom_data.py", line 12, in <module>
    from .data_source import DataSource
  File "/home/nicpa/codes/sisl/src/sisl/viz/data_sources/data_source.py", line 6, in <module>
    from nodify import Node
ModuleNotFoundError: No module named 'nodify'

which does not indicate that nodify is a required package. I think in this case, we should do a more clear message to end-users on how to mitigate this, e.g. do pip install sisl[viz] or simply do pip|conda install nodify.

[pfebrer]

Hmm ok, then I would tell them to pip install sisl[viz]. Installing nodify is something that anyone with minimal python experience would do seeing that error I think.

[zerothi]

Hmm ok, then I would tell them to pip install sisl[viz]. Installing nodify is something that anyone with minimal python experience would do seeing that error I think.

Yes, but that doesn't incorporate conda, I think we could make it simpler for them in some way.

[zerothi]

I added a minimal thing ;)