lephare-photoz / lephare

LePHARE is a code for calculating photometric redshifts.
MIT License
5 stars 1 forks source link

Zphota on my system does not exit if config is not present #202

Closed johannct closed 1 month ago

johannct commented 1 month ago

https://github.com/lephare-photoz/lephare/blame/86fa1daae4fff37c47f6cf44cc0984da5ee55839/tests/lephare/test_zphota.py#L14

Indeed runner.py#L123 just prints a warning out. How come this passes CI unit tests?

johannct commented 1 month ago

Ok calling pytest from the root dir does not do the same as python -m pytest -s tests which is run in the CI

johannct commented 1 month ago

nope... Running pytest triggers the sequence of tests from each file (alphabetical order), and then fail on test_zphota.py, while running pytest tests/lephare/test_zphota.py does not fail....

johannct commented 1 month ago

What I am pretty sure about is that there is not system exit :

In [1]: from lephare import Zphota LEPHAREDIR is being set to the default cache directory: /home/cohen/.cache/lephare/data More than 1Gb may be written there. LEPHAREWORK is being set to the default cache directory: /home/cohen/.cache/lephare/work Default work cache is already linked. This is linked to the run directory: /home/cohen/.cache/lephare/runs/20240501T000334 In [2]: Zphota() WARNING: no config file provided! Out[2]: <lephare.zphota.Zphota at 0x725dd4b74140>

raphaelshirley commented 1 month ago

It throws a SystemExit: 2 for me

johannct commented 1 month ago

in an interactive session?

raphaelshirley commented 1 month ago

Yes in a notebook

raphaelshirley commented 1 month ago

Screenshot 2024-09-10 at 15 10 59

johannct commented 1 month ago

ok we need to sort this difference of behavior out. Obviously it prints the WARNING, so the system exit occurs afterwards, so it has to occur when parsing the args....

raphaelshirley commented 1 month ago

Does this produce SystemExit: 2 on your machine:

import argparse
parser = argparse.ArgumentParser()
parser.add_argument('foo')
args = parser.parse_args()
johannct commented 1 month ago

yes. This does not :

import argparse parser = argparse.ArgumentParser() parser.add_argument('foo', action='store_true') args = parser.parse_args()

and this does not either :

parser.add_argument('--foo')

So it looks like the problem is in the formatting of some arguments?

johannct commented 1 month ago

By the way your system exit above is different : it says something about argument - f not allowed. Can you %tb after the exception occurred on the python session?

raphaelshirley commented 1 month ago

Can we just add raise SystemExit() after the printed warning?

johannct commented 1 month ago

I was expecting to be possible to run without a config file if all the necessary arguments are passed at the command line or as arguments in a python session. It would be better to try to understand what is going on. Especially because on your system it seems to be looking at a kernel configuration. Can you try outside of a notebook?

raphaelshirley commented 1 month ago

If I run it in a python script it prints the warning but doesn't throw a system exit but the test runs fine.

raphaelshirley commented 1 month ago

Bit of a long shot but is None parsed as a string "None" depending on system?

johannct commented 1 month ago

We do not depend on the system : I also have an exit error 2 when I run your snippet in a notebook. I do not know whether @OliviaLynn tried it on a notebook and then added this test in, but you and I are consistent, irrespective of the system : no exit error if run at the command line, exit error when run from a notebook.

What remains to understand is the difference in behavior of pytest depending on how one runs it.... But it is not a big deal. I suggest that we remove this test.

raphaelshirley commented 1 month ago

It's https://github.com/lephare-photoz/lephare/blob/86fa1daae4fff37c47f6cf44cc0984da5ee55839/src/lephare/runner.py#L47 that is breaking in the notebook

johannct commented 1 month ago

yes, but this is because jupyter notebook seems to handle argparse bizarrely. Not super surprising given the fact that it is really not a module meant for notebook, is it? L.47 is exactly where the code is when we run it as an executable

raphaelshirley commented 1 month ago

Ok as long as it is never needed to run in a notebook I am happy to just remove the test and accept that the argument parser has a few system dependent differences with regard to producing a SystemExit.

johannct commented 1 month ago

To go back to the start of this : we do not run unit tests on notebook, so the test of system exit is, no matter what, irrelevant.

raphaelshirley commented 1 month ago

I agree. We can close the issue after removing the test as far as I am concerned.

raphaelshirley commented 1 month ago

We could also replace it with a test that the Warning is printed:

https://pavolkutaj.medium.com/how-to-test-printed-output-in-python-with-pytest-and-its-capsys-fixture-161010cfc5ad

johannct commented 1 month ago

why not, thanks for the pointer, committed in c4cfe6c