Skare3 ci

[javierggt]

Description

This PR makes some changes that are convenient within the CI process:

• add a --root argument to testr so one can run it from a different directory than the config directory
• output a comprehensive machine-readable log file at the end of the run
• include some meta data in log file (start/stop time, system info, package versions)
• add a way to pass options to pytest using an environmental variable.

Documentation can temporarily be found here.

After this PR, testr will not work with ska_helpers version 0.1.1 or earlier.

Note

There is an inherent inconsistency that comes from using testr to begin with. When a pytest test is skipped, all the test cases in there are not reported as skipped. All test suites in there are also not reported. The whole file is reported as a single test case. On the other hand, if it is run, then it is reported as a test suite, and all test cases are reported. This really bugs me.

I can (and will) use some heuristics downstream to figure out whether a given test was actually a test suite. This is not ideal.

Interface impact

This PR introduces a change in behavior. It introduces the --root option, which allows testr to be called from a different directory than its configuration directory.

Before this PR:

• The --packages-dir, --outputs-dir and --regress-dir were implicitly interpreted either as absolute paths, or as paths relative to the configuration directory (same as the working directory).
• The --outputs-subdir was assumed to be a relative path, but a user could actually provide an absolute path.

After this PR:

• --outputs-subdir is gone. outputs-subdir is always given by the get_version script.
• --regress-dir is gone. Now there are two directories within outputs-subdir: logs and regress.
• --packages-dir is gone. It is allways join(outputs-subdir, 'packages')

Testing

• Passes unit tests on MacOS, linux, Windows (at least one required). This package still needs unit tests, but they are not being added in this PR.
• Functional testing. Running with the following argument combinations (of the relevant arguments)
  • none
  • --root
  • --root and --outputs-dir
  • --root $ROOT --test-spec test_spec_Ska.ftp
  • `--root $ROOT --include dea_check --outputs-dir junk2 (for regress)
  • --root $ROOT --test-spec test_spec_SKA3_HEAD

[jeanconn]

With regard to final review, I'm not sure if it is ready without the ska_testr tests done on a spec? I'm also not sure how to run it. Me, I just hacked a local version or run_testr to remove the testr==4.3.1 bits and added this testr to my PYTHONPATH... and a handful of tests now fail (chandra_aca, kadi, mica, annie, the acis checkers).

[javierggt]

yes, and actually... it is failing that one :-) so definitely not ready for final review. Removing the tag.

[javierggt]

It took me a while to figure out. Like all puzzles, the first hurdle is to convince yourself that something is wrong...

I have tracked all test failures to the call to ska_helpers.get_version. It seems to have a side effect of changing the environment. For example, importing kadi causes DJANGO_SETTINGS_MODULE to appear in os.environ. This causes the kadi and chandra_aca failure. I still do not know the exact chain of events.

[javierggt]

so... the question is how to fix it.

• revert changes to os.environ right after the ska_helpers.get_version call.
• change ska_helpers so the environment is never changed:
  • undo changes after getting the version
  • calling the get_version function in a child process (move get_version to child process ska_helpers#11)

One thing that puzzles me is that, these two differ:

In [1]: import ska_helpers
   ...: import os
   ...: ska_helpers.get_version('kadi')
   ...: 'DJANGO_SETTINGS_MODULE' in os.environ
   ...: 
Out[1]: False

In [1]: import ska_helpers
   ...: import os
   ...: ska_helpers.get_version('dea_check')
   ...: 'DJANGO_SETTINGS_MODULE' in os.environ
   ...: 
Out[1]: True

[jeanconn]

I don't immediately understand why having DJANGO_SETTINGS_MODULE set is breaking other things. Do you understand that?

[javierggt]

I have seen this error multiple times in aca_view.

It has to do with the way django initializes, combined with how much stuff a child process inherits. If django is initialized in the parent process, all models and such are created there. Then the child process inherits "something" no which prevents the child process from initializing django again, but it does not inherit the models (hence no maneuvers).

[javierggt]

The failures in the acis code were different. I have not gone to see the chain of events, but they were fixed by the same change in ska_helpers.

[jeanconn]

OK, so the hypothesis is that the environment variable is just another symptom of the same problem (kadi/django already initialized), but having the variable set is not the real problem?

[jeanconn]

I'm mostly trying to figure out if this should be fixed in ska_helpers or if some kind of reasonable change should be made in kadi.

[javierggt]

I have not checked if the acisfp_check failures are because of the same environmental variable. I'm checking now. I think it is, but the final error looks different.

[taldcroft]

Ugh. This has always been a thorn to thread the needle between the kadi web app and kadi package app. Django changed things at some point between 1.6 and 3.0 that made it trickier to do both. See __init__.py in https://github.com/sot/kadi/pull/143/files.

Basically if DJANGO_SETTINGS_MODULE is set before importing kadi then it assumes it is running as a web app and does not import the kadi symbols into the package (doesn't do the from .query import *).

But each time I see this problem I also get confused and have to dig into it again...

[taldcroft]

Looking up the thread, if the child process inherits DJANGO_SETTINGS_MODULE then the problem makes sense.

[jeanconn]

OK, so it sounds to me like @javierggt 's idea to do a fix not in kadi (most likely in ska_helpers in a child process) makes sense for our current use cases. It seems awkward but doable? But now I'm confused about @taldcroft 's last comment with regard to the child process

[taldcroft]

I'm just saying that if, for some reason, D_S_M gets set as an env var in a new process, then using kadi will fail:

(ska3) ➜  ~ env DJANGO_SETTINGS_MODULE='kadi.settings' ipython
Python 3.6.2 |Continuum Analytics, Inc.| (default, Jul 20 2017, 13:14:59) 
Type 'copyright', 'credits' or 'license' for more information
IPython 6.1.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from kadi import events

In [2]: events.manvrs
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-2-2aa9ab524699> in <module>()
----> 1 events.manvrs

AttributeError: module 'kadi.events' has no attribute 'manvrs'

[javierggt]

@taldcroft's comment about the child process just agrees with what I said above.

• The simplest change would be in testr, removing DJANGO_SETTINGS_MODULE or any other change to the environment as it appears. But this is ugly and ad-hoc.
• Requiring that ska_helpers.get_version does not change the environment might be a better way, but do we really want to spawn one process for each function call?
• Changing kadi "might" mean that another package can do something similar.

So there is no perfect solution.

[taldcroft]

Shouldn't there be a way to get the version without importing (and avoid all possibility of side effects?).

https://stackoverflow.com/questions/4693608/find-path-of-module-without-importing-in-python

>>> import importlib
>>> spec = importlib.util.find_spec("threading")
>>> spec.origin
'/usr/lib64/python3.6/threading.py'

The only reason I can see now for the import is to get the module __file__.

[javierggt]

I think that's better. I was also wondering if it was possible to not import. I think that will fix it.

[javierggt]

some of the failing tests passed. running all test_spec_SKA3_HEAD now.

[taldcroft]

My hope is that there is a solution so that one can do import <package>; <package>.test() to test the installed package and get a clean result that is free of "expected" or ignorable warnings, but still shows real warnings (e.g. a deprecation warning coming from numpy). This should happen without requiring any setup of env vars (which requires knowing or remembering there is an option and then finding the documentation about that).

I tried adding -Wignore::PytestDeprecationWarning to the default list of ignored warnings but I got an exception that PytestDeprecationWarning is not defined. Not sure if I was doing something wrong or that warning class needs to get imported somehow.

[javierggt]

I specified the junit_family option on the command line and it works. I tried it in shiny, and I was surprised to see it also worked in the older pytest version in ska3, so no conditional required.

[javierggt]

@taldcroft and @jeanconn, I think this is ready.

The one thing I am not sure I did to your liking was the --regress-dir option. I do not understand the logic behind the previous directory layout, and this version by default will put log and regress directories side by side for each run, within each run's directory (the one pointed to with the "last" link).

This is documented in the sphinx documentation linked above.

[taldcroft]

Now I recall the rationale for the original structure, which is that it makes it relatively easy to diff two versions using diff -r, for example:

cd regress
diff -r version1 version2

The issue with your new structure is that version1 and version2 include all the test outputs alongside the regress outputs.

[javierggt]

I see. With my changes you would have to do

diff -r version1/regress version2/regress

[taldcroft]

Ah, no problem them. I had guessed without looking that it would be something like version / package / {log, regress}. But version / {log, regress} / package is just fine.

[taldcroft]

My main comment now is considering this an opportunity to simplify. I have only myself to blame, but the user interface seems "over-configurated" and I think we could reduce to just two directory options:

--config-dir: directory containing configuration files (test specs, `packages`, and `get_version_id`).  
  Default = '.'
- output-dir: directory containing all the outputs in fixed structure `version_id / {logs, regress} / package`.
  Default='outputs'

Otherwise this looks fine. I have not gone through the code in fine detail but if there is a problem we can fix it.

[javierggt]

yes, I agree with that. Will commit in a bit so we are done with this.

[javierggt]

but note that this change would amount to removing arguments that are already optional.

[taldcroft]

🎉