transientskp / tkp

A transients-discovery pipeline for astronomical image-based surveys
http://docs.transientskp.org/
BSD 2-Clause "Simplified" License
19 stars 14 forks source link

Test data not being collected from github #601

Closed AntoniaR closed 1 year ago

AntoniaR commented 1 year ago

When setting up for testing TraP, we download the test suite data by running (from tkp folder):

git submodule init
git submodule update

With the second command, I get the error:

fatal: reference is not a tree: 60d8ab7018065ab17aa1a37feb954741e15c1fbd
Unable to checkout '60d8ab7018065ab17aa1a37feb954741e15c1fbd' in submodule path '../tests/data'

This means that files are missing when running python runtests.py

HannoSpreeuw commented 1 year ago

Thanks, which branch are you using?

AntoniaR commented 1 year ago

I have now switched to the correct branch (converted_to_python3_with_sourcefinder_from_pyse_repo) and still missing the data.

(Python3.6Env) antoniar@struis:~/TraP_py3/tkp$ git submodule update
fatal: reference is not a tree: 60d8ab7018065ab17aa1a37feb954741e15c1fbd
Unable to checkout '60d8ab7018065ab17aa1a37feb954741e15c1fbd' in submodule path 'tests/data'
jdswinbank commented 1 year ago

The problem here is that the submodule reference in tkp points to a commit that doesn't exist in trap-test-data.

You can verify this for yourself by going to https://github.com/transientskp/tkp/tree/converted_to_python3_with_sourcefinder_from_pyse_repo/tests and clicking on the link to data @ 60d8ab7 — you'll get a “404 not found” error.

@HannoSpreeuw , you changed this reference from 4f0764 to 60d8ab on this commit; that's where the problem originates. Most likely, 60d8ab does exist in your own version of trap-test-data, and all you need to do is push it to GitHub.

jdswinbank commented 1 year ago

(By the way, I fully support the effort to migrate from Python 2 to Python 3, but are you aware that Python 3.6 is also obsolete? See https://endoflife.date/python. Unless you have really special reasons to stick with 3.6, I suggest moving to 3.10 or 3.11...)

AntoniaR commented 1 year ago

Oh wow, I'm already out-of-date before I even get my head around python 3.6 :D Nevermind, I'll try installing 3.11 and see how far the code gets.

HannoSpreeuw commented 1 year ago

(By the way, I fully support the effort to migrate from Python 2 to Python 3, but are you aware that Python 3.6 is also obsolete? See https://endoflife.date/python. Unless you have really special reasons to stick with 3.6, I suggest moving to 3.10 or 3.11...)

Thanks for that comment. Reasons described here. The problem came from astropy not working for TraP with Python 3.10.

HannoSpreeuw commented 1 year ago

The problem here is that the submodule reference in tkp points to a commit that doesn't exist in trap-test-data.

You can verify this for yourself by going to https://github.com/transientskp/tkp/tree/converted_to_python3_with_sourcefinder_from_pyse_repo/tests and clicking on the link to data @ 60d8ab7 — you'll get a “404 not found” error.

@HannoSpreeuw , you changed this reference from 4f0764 to 60d8ab on this commit; that's where the problem originates. Most likely, 60d8ab does exist in your own version of trap-test-data, and all you need to do is push it to GitHub.

All right, let me fix that asap. Currently, I get, in tkp/tests/data/, from git status:

HEAD detached from 4f07644
Untracked files:
  (use "git add <file>..." to include in what will be committed)
  ....
nothing added to commit but untracked files present (use "git add" to track)

What should I do?

jdswinbank commented 1 year ago

Astropy version 5.0 and later should support Python 3.10 — that's explicitly tested in CI. Older versions might have problems.

jdswinbank commented 1 year ago

What should I do?

There are a few different options, depending on the desired end state.

If changes to trap-test-data was unintentional, you can simply change the reference to it in the tkp repository so that it points back to 4f0764. This is a change in the tkp repository. Something like:

$ cd tkp
$ cd tests/data
$ git checkout 4f0764
$ cd ../..
$ git commit tests/data -m "Use tkp-test-data version 4f0764"  # note that this commit is in the tkp repo
$ git push

Alternatively, perhaps you have previously committed changes to trap-test-data that you want to use --- presumably as commit 60d8ab --- then you can simply push it to GitHub. Something like the following should work:

$ cd trap-test-data
$ git checkout master  # note that this checkout is in the trap-test-data repo
$ git show  # check that the most recent commit is really 60d8ab, and if so...
$ git push origin master

Finally, it may be that you actually need all the files that are currently “untracked” in your trap-test-data repo. In that case, you need to both add, commit, and push them, and then also update tkp to point to the new repo. So something like:

$ cd tkp/tests/data  # in the trap-test-data repo within the tkp tree
$ git add -A
$ git commit -m "Add new files"
$ git push origin master
$ cd ../..  # Now we're in the tkp repo itself
$ git add tests/data 
$ git commit -m "Use new trap-test-data"
$ git push

Unfortunately, I can't tell you which of those is correct — it really depends on how we got to the current state, which I think only you can know! :-)

HannoSpreeuw commented 1 year ago

Astropy version 5.0 and later should support Python 3.10 — that's explicitly tested in CI. Older versions might have problems.

Sure, but the problem with Python 3.10 was about installing TraP. Using Python 3.9 resulted in a different installation problem for TraP.

jdswinbank commented 1 year ago

Sure, but the problem with Python 3.10 was about installing TraP.

I guess I'm confused, because this comment seems pretty clear that you were having troubles with Astropy — 

combining Python 3.10 with astropy did not work

But I don't think my confusion is important! Astropy shouldn't be a problem in and of itself! :-) If you need a hand getting to the bottom of the issue, feel free to give me a shhout.

HannoSpreeuw commented 1 year ago

Yeah, just trying to explain the background of me choosing Python 3.6 three months ago. Installing Trap with Python 3.10 apparently caused a problem coming from Astropy that I thought I could not fix easily. Now, in hindsight, I realise it came from TraP passing an object that had been deprecated in Python 3.10 - or rather moved to a module within collections - on to an Astropy module. Anyway, I wanted to achieve a successful Python 3 install of TraP as soon as possible.

HannoSpreeuw commented 1 year ago

What should I do?

There are a few different options, depending on the desired end state.

Thanks John, for this extensive explanation, very helpful!

I am trying to recollect what I must have done three months ago. Applying the two stages of futurize recursively must have affected also the contents of tests/data, since there are not only data in that directory, but also a Python script that generates a FITS image: sourcefinder/simulations/make_map_for_unit_testing_deconvolve_from_clean_beam.py

Btw, I just noticed that this particular part of the Python 2 to 3 conversion had also been taken care of by @gijzelaerr seven years ago.

I guess what happened is that my IDE (VS Code) notified me that I should commit those changes, which I probably did.

HannoSpreeuw commented 1 year ago

I noticed that commit 4f0764 is from 2016

HannoSpreeuw commented 1 year ago

In tests/data, git checkout master gives

Warning: you are leaving 1 commit behind, not connected to
any of your branches:

  60d8ab7 Leftover change from  futurize --stage1 or futurize --stage2 in the data directory.

If you want to keep it by creating a new branch, this may be a good time
to do so with:

 git branch <new-branch-name> 60d8ab7

Switched to branch 'master'
Your branch is up to date with 'origin/master'.
HannoSpreeuw commented 1 year ago

So I guess I must have forgot updating the reference in the TraP project. My changes in tests/data can be omitted because they were already taken care of seven years ago and users can checkout the python3 branch from trap-test-data.

So I will go for the first of the three options that John has described.

@AntoniaR Could you please git pull on the converted_to_python3_with_sourcefinder_from_pyse_repo branch and tell us if the problem has been fixed?

AntoniaR commented 1 year ago

Yes, this issue has now been solved. Though I still seem to have issues with passing the database details along.