search

ratt-ru / CubiCal

A fast radio interferometric calibration suite.
GNU General Public License v2.0
18 stars 13 forks source link

make pypi compatible #446

Closed gijzelaerr closed 3 years ago

ratt-priv-ci commented 3 years ago

Can one of the admins verify this patch?

bennahugo commented 3 years ago

I think we need to test this first - there was a reason to have it pinned to 0.6.1.

I also think montblanc requires you to first install at least the CPU version of tensorflow - note it has to be 1.8.0

python3 -m pip install tensorflow==1.8.0
bennahugo commented 3 years ago

If I recall the code requires the old SPI specification so we need 0.6.1 unfortunately. Can you roll this back to that version (ie. <=0.6.1)

gijzelaerr commented 3 years ago

can someone then at least push 0.6.1 to pypi?

bennahugo commented 3 years ago

@sjperkins can you do this?

On Sun, 14 Mar 2021, 15:33 Gijs Molenaar, @.***> wrote:

can someone then at least push 0.6.1 to pypi?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ratt-ru/CubiCal/pull/446#issuecomment-798907601, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6WZ64LG535YY74FRHDTDS3LJANCNFSM4ZE4H3EQ .

JSKenyon commented 3 years ago

I am unsure what changed in Montblanc - if someone can tell me, perhaps I can change the code to be compatible with the latest version. Failing that, I think it is relatively safe to hard pin the Montblanc version once 0.6.1 is on PyPI.

sjperkins commented 3 years ago

@sjperkins can you do this?

I''m not going to give it immediate focus. From memory the issue was Tensorflow as a Montblanc build dependency -- this required a manual installation of tensorflow to build Montblanc (and hence Cubical) from source.

I think there may now be PEP517 concerns too. https://github.com/ska-sa/montblanc/pull/272. @bennahugo what was the requirement here for 0.6.4? DDFacet? Has anyone tried Cubical with 0.6.4.

@gijzelaerr you have "owner" on the pypi project. If you need this done quickly I would suggest trying to see if you can upload a 0.6.1 source tarball, but this may not lead to everything just working.

gijzelaerr commented 3 years ago

@sjperkins good point, thanks! upload 0.6.1 to pypi.

bennahugo commented 3 years ago

DDFacet works with the latest version @sjperkins. However one of the commits since 0.6.1 is a change in the SPI specification, which makes me think it will break cubical.

bennahugo commented 3 years ago

I think the user will just need to install tensorflow manually prior to installing cubical. It should crash out with a nice message to the user though so this I don't think is too much to ask from the user.

sjperkins commented 3 years ago

@sjperkins good point, thanks! upload 0.6.1 to pypi.

I wouldn't do it straight away because once that 0.6.1 tarball is up, it can't be replaced. Perhaps see if you can pip install Cubical using the 0.6.1 source tarball somehow.

gijzelaerr commented 3 years ago

i mean uploadED, it is already uploaded.

sjperkins commented 3 years ago

i mean uploadED, it is already uploaded.

OK! On second thoughts it wouldn't make sense to redo the tag contents now that it's been in the wild so long, so I think this is fine.

bennahugo commented 3 years ago

ok to test

o-smirnov commented 3 years ago

The spi specification is just a change in how the spectral polynomial is expressed, no? Can't we just have both, and an option to switch between? I mean ideally it's just two different kinds of spectral models, and the codebase ought to support both (as should codex).

sjperkins commented 3 years ago

Folks, a gentle reminder to:

  1. check your github repo issue history and use descriptive searchable titles to make this easy.
  2. Use the git blame feature to search through file changes (in this case, setup.py).

Using the above tools to do search through history it is possible to conclude that: Cubical has been using the "new" SPI functionality for a while:

Thus, 0.6.4 should work.

Other threads worth revisiting:

  1. https://github.com/ratt-ru/CubiCal/issues/273
  2. https://github.com/ratt-ru/CubiCal/pull/275
sjperkins commented 3 years ago

It looks from those threads that the idea was that Montblanc would be an optional dependency.

bennahugo commented 3 years ago

There is this commit: https://github.com/ska-sa/montblanc/commit/e8b2e7140e119f0d9e9caeb16f3fa66ec71406be after the 0.6.1 release that I'm not sure of. Lets first get it pinned to 0.6.1 then we can worry about making potentially breaking changes.

sjperkins commented 3 years ago

There is this commit: ska-sa/montblanc@e8b2e71 after the 0.6.1 release that I'm not sure of. Lets first get it pinned to 0.6.1 then we can worry about making potentially breaking changes.

0.6.1 was probably the checkpoint release before the spectral index handling changed.

sjperkins commented 3 years ago

0.6.1 was probably the checkpoint release before the spectral index handling changed.

Correction, 0.6.1 tags https://github.com/ska-sa/montblanc/pull/244/commits/547008faa46d5798f682d9d00597351a67f1915e in https://github.com/ska-sa/montblanc/pull/244.

0.6.2 was tagged with the merge of #244 into master.

They are functionally identical: https://github.com/ska-sa/montblanc/compare/0.6.1..0.6.2

sjperkins commented 3 years ago

See you for the same discussion next year 😉

sjperkins commented 3 years ago

DDFacet's montblanc dependency is also >= 0.6.1

https://github.com/cyriltasse/DDFacet/blob/5289b7adb347826da00e68f14bba5765d29b8f3c/setup.py#L223

bennahugo commented 3 years ago

Yes sure, but this has been fixed on 0.6.1 - it may work with 0.6.3 but we have to test it first by hand before we bump it up. I have changed the PR to <= 0.6.1 for now. I also need to remove the py2 tests (which is why this didn't pass yet)

On Tue, 16 Mar 2021, 08:57 Simon Perkins, @.***> wrote:

DDFacet's montblanc dependency is also >= 0.6.1

https://github.com/cyriltasse/DDFacet/blob/5289b7adb347826da00e68f14bba5765d29b8f3c/setup.py#L223

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ratt-ru/CubiCal/pull/446#issuecomment-800006182, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6TNFVWZXIC5VYJLHH3TD36O7ANCNFSM4ZE4H3EQ .

JSKenyon commented 3 years ago

I think it is actually safe to change the version. I believe I actually already did in another PR which is awaiting review. Whilst I cannot say my testing was exhaustive, it certainly worked. I believe @ulricharmel has been using that branch without issue. So 0.6.4 should be fine.

JSKenyon commented 3 years ago

I am happy to merge this - any concerned parties please voice your opinions before the end of the day, otherwise I will push the button.

JSKenyon commented 3 years ago

@gijzelaerr this has been merged if you want to try uploading to PyPI.