Closed pfebrer closed 5 months ago
I don't understand why CI fails, it fails even before installing python :sweat_smile:
It could be #755, lets wait a day or so... Otherwise I will probably revert it...
Actually, it couldn't since that is a different work-flow... Sorry for the blurp!
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
Now the problem is there because nodify
isn't a package (it can't pip install it?)
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.
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 :)
rebase, and lets try again!
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 87.25%. Comparing base (
8d6da59
) to head (2831533
). Report is 1 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Sorry for another go here. Could you rebase, I am trying to add another test that checks without viz
. :)
The tests shows it should be more hidden, i.e. Since it is optional there needs to be a fallback on it.
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.
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
?
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 insidesisl/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.
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.
I think a better solution would be to have a
TEST_VIZ
environment variable or something like that, which should makepytest
not gather tests insisl/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 topip 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 waypytest
works (i.e. you want pytest to show that these tests exist but are skipped). I thinkpytest
shouldn't determine the way the package works. So instead I would propose that whatever hack that is needed forpytest
to show the logs that you want (e.g. mockingsisl.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. ;)
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? :sweat_smile:
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
.
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 thatsisl
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 attemptedsisl.viz
manages to get to the part where env variables are registered, which is before trying to importnodify
, 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 importsisl.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.
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 ?
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.
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.
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
.
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.
Hmm ok, then I would tell them to
pip install sisl[viz]
. Installingnodify
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.
I added a minimal thing ;)
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 :)