dingo-gw / dingo

Dingo: Deep inference for gravitational-wave observations
MIT License
60 stars 20 forks source link

Dingo Pipe with Python>=3.6 #145

Closed nihargupte-ph closed 1 year ago

nihargupte-ph commented 1 year ago

https://github.com/dingo-gw/dingo-devel/blob/d2285d6681c5f37f9e498fbb93b2a00805cd17fe/dingo/gw/pipe/nodes/plot_node.py#L8

Only works with python 3.9: https://stackoverflow.com/questions/66683630/removesuffix-returns-error-str-object-has-no-attribute-removesuffix

In setup.py we have >=3.6.

stephengreen commented 1 year ago

I think we can safely require python 3.9. What do others think? @max-dax @jonaswildberger @mpuerrer

nihargupte-ph commented 1 year ago

Alternative is to change removesuffix("_merge") to replace("_merge", "")

max-dax commented 1 year ago

I prefer Nihar's suggestion. Not supporting 3 python versions because of one line that can easily be replaced seems a bit unnecessary, no?

stephengreen commented 1 year ago

It's not like we really support python 3.6 anyway. (Have we actually tested it?) E.g., our requirement pytorch only supports 3.7, and soon 3.8:https://pytorch.org/blog/deprecation-cuda-python-support/On Feb 28, 2023 08:27, max-dax @.***> wrote: I prefer Nihar's suggestion. Not supporting 3 python versions because of one line that can easily be replaced seems a bit unnecessary, no?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

stephengreen commented 1 year ago

Happy to change that line though. But we should think if we really want to claim to support certain Python versions.On Feb 28, 2023 08:38, Stephen Green @.> wrote:It's not like we really support python 3.6 anyway. (Have we actually tested it?) E.g., our requirement pytorch only supports 3.7, and soon 3.8:https://pytorch.org/blog/deprecation-cuda-python-support/On Feb 28, 2023 08:27, max-dax @.> wrote: I prefer Nihar's suggestion. Not supporting 3 python versions because of one line that can easily be replaced seems a bit unnecessary, no?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

max-dax commented 1 year ago

I see. Some of my environments are python 3.8, so at least for that version it is tested. Happy either way though.

stephengreen commented 1 year ago

I think we should exclude 3.6 in any case because dicts are not ordered. Not sure about 3.7 or 3.8. Some of the typing we use might only be in newer versions.On Feb 28, 2023 08:41, max-dax @.***> wrote: I see. Some of my environments are python 3.8, so at least for that version it is tested. Happy either way though.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

max-dax commented 1 year ago

Oh that's a good point, 3.6 definitely should be excluded then. Re. 3.8, as I said it works fine for me, but let's just migrate to >= 3.9 then if it makes things easier.

nihargupte-ph commented 1 year ago

Right my environments are 3.8 as well, but I agree probably 3.6 should be removed.

mpuerrer commented 1 year ago

If >= 3.9 has not been exhaustively tested yet, we could stick with >= 3.8. Otherwise >= 3.9 sounds fine.

stephengreen commented 1 year ago

Okay, so we are settled on supporting 3.9 and 3.10, and not 3.6 or 3.7.

Questions:

nihargupte-ph commented 1 year ago

Right, I think 3.11 is untested. I would advocate for 3.8 support also since all my venvs which are tied to different versions of LALsuite work with 3.8 and I've not tested it for 3.9. If we are happy with 3.8 and 3.9 I can make a PR and close this

stephengreen commented 1 year ago

Ok sounds like a good reason. Please just make the needed change then. Thanks.

stephengreen commented 1 year ago

To summarize, we will support 3.8, 3.9, 3.10 for now.