make pypi compatible

[gijzelaerr]

No description provided.

[ratt-priv-ci]

Can one of the admins verify this patch?

[bennahugo]

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]

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]

can someone then at least push 0.6.1 to pypi?

[bennahugo]

@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 <#446 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4RE6WZ64LG535YY74FRHDTDS3LJANCNFSM4ZE4H3EQ> .

[JSKenyon]

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]

@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. ratt-ru/montblanc#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]

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

[bennahugo]

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]

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]

@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]

i mean uploadED, it is already uploaded.

[sjperkins]

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]

ok to test

[o-smirnov]

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]

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:

• 9afe9b0#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R156

Thus, 0.6.4 should work.

Other threads worth revisiting:

2. do we really need a hard montblanc/tensorflow dependency? #273
3. Fixes #273 #275

[sjperkins]

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

[bennahugo]

There is this commit: ratt-ru/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.

[sjperkins]

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]

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

Correction, 0.6.1 tags ratt-ru/montblanc@547008f in ratt-ru/montblanc#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]

See you for the same discussion next year 😉

[sjperkins]

DDFacet's montblanc dependency is also >= 0.6.1

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

[bennahugo]

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 <#446 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4RE6TNFVWZXIC5VYJLHH3TD36O7ANCNFSM4ZE4H3EQ> .

[JSKenyon]

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]

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]

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