Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Dec 7, 2023

Conversation

ddobie
Copy link
Contributor

@ddobie ddobie commented Dec 6, 2023

Fix #688.
Fix #686

@ddobie
Copy link
Contributor Author

ddobie commented Dec 6, 2023

@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 ddobie changed the title Added new regression data download method Add new regression data download method and update github actions ubuntu version Dec 6, 2023
@ddobie
Copy link
Contributor Author

ddobie commented Dec 6, 2023

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
Copy link
Contributor

ajstewart commented Dec 6, 2023

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
Copy link
Contributor Author

ddobie commented Dec 7, 2023

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
Copy link
Contributor

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: ajstewart@5d58a7c

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
Copy link
Contributor Author

ddobie commented Dec 7, 2023

Sounds good to me!

@ddobie ddobie requested a review from ajstewart December 7, 2023 10:54
Copy link
Contributor

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to consider changing the python version in the toml as well.

No action required with this but as I have dealt with it a recently: the testing on poetry projects is always a bit dubious because of the mismatch between the poetry lock and installing via pip which ignores it. It does cause grief I have to admit.

Pipeline is not a package so it can't be locked to versions but a way around it is to have poetry export the requirements file (often done as a pre-commit step), and then users / workflows can install how they want with the same locked versions'.

However, this also causes pain with dependabot as that will not update the requirements file alongside the poetry ones (it considers pip and poetry to be a python dependency system and not separate ones).

The advantage of using poetry in the tests is worth it as now it will actually test what we have defined and is reproducible. Including the dependabot PRs.

So in the end it's best to just use poetry and be consistent with it and use it everywhere. Users can export their own requirements file if they really want to. I think the docs might say it already but pip install the pipeline has the potential to give unexpected results.

If only python had a fully standardised system 😄

@@ -10,13 +10,14 @@ echo

TMPFILE=`mktemp`
TMPDIR=`mktemp -d`
wget "https://cloudstor.aarnet.edu.au/plus/s/xjh0aRr1EGY6Bt3/download" -O ${TMPFILE}
wget "http://www.physics.usyd.edu.au/~ddob1600/vast-pipeline/pipeline-test-data.zip" -O ${TMPFILE}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may also not be a permanent solution?


echo "Unzipping archive to ${TMPDIR} ..."
unzip -q -d ${TMPDIR} ${TMPFILE}

echo "Moving data to relevant directory..."
mv ${TMPDIR}/VAST_PIPELINE_TEST_DATASET/* $(pwd)/.
#mv ${TMPDIR}/VAST_PIPELINE_TEST_DATASET/* $(pwd)/.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#mv ${TMPDIR}/VAST_PIPELINE_TEST_DATASET/* $(pwd)/.

@ddobie ddobie merged commit 541a04f into dev Dec 7, 2023
4 checks passed
@ddobie ddobie deleted the fix-iss-688 branch December 7, 2023 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Cloudstor source for regression tests dataset returning 403 Outdated workflow Ubuntu versions
2 participants