search

askap-vast / vast-pipeline

This repository holds the code of the Radio Transient detection pipeline for the VAST project.
https://vast-survey.org/vast-pipeline/
MIT License
8 stars 3 forks source link

Add new regression data download method and update github actions ubuntu version #699

Closed ddobie closed 10 months ago

ddobie commented 10 months ago

Fix #688. Fix #686

ddobie commented 10 months ago

@ajstewart we've finally found a hosting solution for the test data, but either the tests or the data seem to be broken. Any thoughts?

ddobie commented 10 months ago

Just set up a test environment on ada and ran all the tests with no errors. So maybe it's a problem with the github workflows?

ajstewart commented 10 months ago

My first check would be how the python dependencies are installed in the test workflow, which seems to be just with a pip install . and the versions are not the same that's in the poetry.lock.

It has been so long that it might be installing different versions with unexpected results.

So try changing the test workflow to also install using poetry.

It looks like the data is making it there, but something is crashing in the pipeline runs.

ddobie commented 10 months ago

Okay I've got the 3.8 and 3.9 tests working.

The 3.10 tests are breaking because of the version of llvm-lite, which is set to 0.34 in my poetry.lock file, but should be >0.38 in order to support 3.10 (https://llvmlite.readthedocs.io/en/latest/release-notes.html#v0-38-0-january-13-2022). However, llvm-lite isn't a direct dependency, it's required by numba which in turn is required by something else (I haven't gone back through and traced it).

What's the best practice here? Specifying it in the pyproject.toml?

ajstewart commented 10 months ago

This sounds familiar, in fact it looks like I dealt with the exact same when I was upgrading the dependencies in my version with the upgrades: https://github.com/ajstewart/vast-pipeline/commit/5d58a7cc7600eba3817c5f78dabdf32d2342b957

Seems like the pipeline never really supported 3.10 in the first place? You could just remove it for now to get this through. I don't think it was ever properly tested.

Like I did above I would shift the minimum version to 3.10 in a future update. A PR could be opened for that branch when you are ready for it.

ddobie commented 10 months ago

Sounds good to me!