Use httpx for server connection, some restructuring and cleanup

[teutoburg]

Migrate the whole package from requests to httpx, which nicely fits the kind of web requests we're doing here and is easily expandable, should we ever need more advanced functionality.

Additionally, the location of the cached files is changed to something more reasonable than inside the package directory (still supports scopesim_data and an environ variable). Dropped caching of parameter JSON file, I don't think this was used anywhere at all. General working principle of the cache (hash of input parameter combination) is unchanged.

In the course of that, I also changed some other things:

• use pathlib.Path instead of os.path, also in the tests
• use logging instead of printing, except in methods that have "print" in the name
• add a base class for the two previous classes in core to inherit from, related to common request operations
• rename various parameters to make the two now-sibling classes more similar
• make these classes callable, because they really only have one main use, and one even had a .call() method before...
• some improvements of the tests, most notably use actual fixtures instead of just calling global instances "mocks"
• add more docstrings
• multiple minor changes and improvements, formatting

Several aspects of this still need some attention, but it's a big step forward. I also bumped the development version number in the pyproject.toml file, because the structure changed significantly enough that this should be a minor version increase once we release it...

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (85f0faf) 99.02% compared to head (2048146) 99.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   99.02%   99.08%   +0.05%     
==========================================
  Files           2        2              
  Lines         205      218      +13     
==========================================
+ Hits          203      216      +13     
  Misses          2        2              

Files	Coverage Δ
skycalc_ipy/tests/test_core.py	100.00% <100.00%> (ø)
skycalc_ipy/tests/test_ui.py	98.80% <100.00%> (+0.06%)	⬆️

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

[hugobuddel]

While these changes themselves are fine, we should first determine what path we want to take with skycalc_ipy. The change of ETC server bit us unexpectedly, and it was entirely our own fault. The changes in this PR can make it both easier and harder to prevent similar problems in the future, depending on our plan.

We based skycalc_ipy on skycalc-cli 1.1. Then ESO changed the server, and the units, in 1.2 year ago. skycalc-cli is at 1.4 now, without us even checking what changed, let alone incorporating those changes in skycalc_ipy.

I see three ways of keeping skycalc_ipy in sync with skycalc-cli:

• Keep our codebase entirely separate and manually make sure it is in sync with ESO's code. Merging this PR would make comparing against ESO's code harder because the code bases will have diverged.
• Use skycalc-cli as a dependency of skycalc_ipy, but only for things like using the correct URL. This PR would makes sense, because the codebases would be decoupled.
• Use skycalc-cli as a dependency and make skycalc_ipy a very thin wrapper around existing functionality in ESO's code. Then this PR would probably not be necessary as most of the code in this project would hopefully be unnecessary.

My personal preference is the last option: making skycalc_ipy a thin wrapper around skycalc-cli; however, I'm not sure whether that is feasible. If not, then I'd prefer the middle option to at least have the URL correct, and we should merge this. The first option is what we do now, and if that is indeed our plan, then we might be better of not merging this.

So for me it would only be possible to advice on whether to merge this if we know what our long term plan is.

[hugobuddel]

Dropped caching of parameter JSON file, I don't think this was used anywhere at all.

The parameters were cached because without them it is impossible to tell which FITS files are which. Say you have cached too many requests and want to prune the ones you don't care about anymore, then you could use the cached json files to determine which queries you are no longer interested in anymore.

On the one hand I'd prefer it if the JSON files are kept in the cache. On the other hand, I have not ever performed such pruning as described above, so maybe it doesn't really matter.

Maybe the parameters are also included in the FITS files somehow though, then it would be better to just use those.

[teutoburg]

Is skycalc-cli open source? I cannot find any source code for it, only the website.

[hugobuddel]

The source package can be downloaded from PyPI: https://pypi.org/project/skycalc-cli/ , it seems to be MIT licensed.

I haven't looked closely at the source, only to verify the new URL. So I don't know how close it is to our source code, or how extendable it is, or anything really.

[teutoburg]

Maybe the parameters are also included in the FITS files somehow though, then it would be better to just use those.

Just checked, they are, under Input parameters in the header. In a somewhat weird way, but usable.

[teutoburg]

I had a quick look at the source code of skycalc-cli. Our own docstring in core.py used to read "This modele was taken (mostly unmodified) from skycalc_cli version 1.1.". This seems to be very much true, it is almost identical to how our code was before I started messing with it from #18 onwards. Indeed the (new) server URL is still as hardcoded as always, and exists twice, both in AlmanacQuery and in SkyModel.

That whole skycalc-cli package seems very crudely done tbh. The current version (I guess that part was added after we "forked") includes a method SkyModel.test() as a form of unittest I guess. At least I found print("Note: skycalc_cli v.1.4 output wavelength unit is nm (it is μm in v.1.3)"), maybe we would have seen that in out failing tests elsewhere. We would certainly see it now every time a request is made, if we make a thin wrapper around skycalc-cli. Might be cumbersome to pull off the caching thing for a wrapper.

Extracting the server URL from skycalc-cli could be done with skycalc-cli.skycalc.AlmanacQuery(indic).almserver, but since it's an instance attribute, we'd need to instantiate it with a valid indic, that all seems very ugly, especially with so much going on in that __init__ method. Getting the URL from skycalc-cli.skycalc.SkyModel().server doesn't need an input dict and has much less going on in __init__, but then we're back at printing that unit message all the time. I know it's possible to hack around that, I did that here to deal with hmbp. Might be asking for trouble though...

Another motivation for these changes was the way skycalc-cli and indeed all of astropy handles downloads and caching. It's causing us quite a bit of trouble with randomly failing tests and is generally a pain to work with, while there are nice modern well-maintained alternatives for such operations.

I don't have a perfect answer to this whole situation either. Let's keep this PR on hold for the moment, it's not really hindering anything else, nor are we likely to make any changes elsewhere that would conflict with this, so there's no hurry.

[hugobuddel]

Thanks for checking the skycalc-cli code. I'm now in favor of merging this and breaking away from skycalc-cli, because

• It seems quite cumbersome to rewrite skycalc-ipy to be a thin wrapper, because it would hamper the improvements we'd like to see.
• Using skycalc-cli just to fetch the URL might actually be a bad idea. They (probably) changed the URL because they also changed the service itself, in particular the units. So we cannot blindly use a new URL anyway.

So we will just have to live with it that we might need to update it occasionally. Luckily we have nightly tests to catch problems.

We don't have to merge it now, but on the other hand, I'm not sure what we would be waiting for.