Upgrade python minimum to 3.8 and tidy up packaging

[lsetiawan]

Overview

This PR updates the python minimum requirement to 3.8 syncing up with xarrays. This resolves #603. Also, some unneeded packages has been deleted from requirements. The setup.cfg has now been filled with all the package requirements even the optional ones. Now any requirements-x.txt files are pretty much redundant.

[emiliom]

Thanks for doing this!

Now any x-requirements.txt files are pretty much redundant.

Did you mean requirements-x.txt? Currently we only have requirements.txt and requirements-dev.txt. While requirements-dev.txt may now be redundant for the CI, it's still required for building local dev conda environments. Given that, having dev requirements listings duplicated in setup.cfg and requirements-dev.txt sounds like a recipe for divergence in the future. Do we lose anything significant by having dev requirements only in requirements-dev.txt?

[codecov-commenter]

Codecov Report

Merging #604 (78b4289) into dev (90a524a) will not change coverage.
The diff coverage is n/a.

❗ Current head 78b4289 differs from pull request most recent head 4dad804. Consider uploading reports for the commit 4dad804 to get more accurate results

@@           Coverage Diff           @@
##              dev     #604   +/-   ##
=======================================
  Coverage   79.18%   79.18%           
=======================================
  Files          41       41           
  Lines        3623     3623           
=======================================
  Hits         2869     2869           
  Misses        754      754           

Flag	Coverage Δ
unittests	79.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[lsetiawan]

Do we lose anything significant by having dev requirements only in requirements-dev.txt?

For dev development, one can do pip install -e .[dev] to get all the required packages as well as the dev dependency. I mean, we can just remove this optional extra packages configuration from setup.cfg and just allow make users do an install from requirements-dev.txt as shown in the docs or update the docs. I was thinking of cleaning up the requirements-x.txt files if we have all the configuration in setup.cfg. One of the "proper" use of the requirements.txt would be for a freezed environment for testing purposes, so maybe we can go that route for development?

[lsetiawan]

Okay. I've updated this to be that dev related requirements are not in setup.cfg to make it cleaner as in what is the barebone package dependencies, so one has to install from requirements-dev.txt for anything that is related to dev and the docs/requirements.txt for docs stuff. Now everything is separated. Also I've moved some settings for black and flake8 to their proper places, now it seems like it's actually expanding the line length! We set this at 100, is this still okay? PEP8 standard is actually 79, black style standard puts it at 88 by default, which I think what was happening.

[emiliom]

Thanks!! We can discuss these tradeoffs (requirements-dev.txt vs setup.cfg vs ...) some other time. I like this being a narrower PR at this time, focused on the >=3.8 change.

On line length, I personally like a length of 100

[emiliom]

Oh, and I can't fully review this PR yet (42 files changed?! Though it looks like many of them are just line lengths). I can get to it maybe by tonight, otherwise tomorrow.

[lsetiawan]

Yea sorry for the blow up in files... that was because of line length fix by pre-commit! Other than that, looks like all tests are passing 😄

[emiliom]

I ended up being able to review this now 😄 . I went through the line-length changes quickly (there are too many!). See my inline comments elsewhere.

[leewujung]

I have a not substantial comment to add, that the PR title should change 😁 Now it is a wayyy bigger PR than what was originally intended.

[emiliom]

I think this is ready to merge, right @leewujung ?

[leewujung]

Yeah I think this is ready to merge. It would be nice to transfer some of the detailed descriptions from here to the Contributing to echopype page, or we can have an infrastructure page.

@emiliom there are some open questions you had above. Do you want those answered before merging? Or take them to another issue?

[emiliom]

@emiliom there are some open questions you had above. Do you want those answered before merging? Or take them to another issue?

Right, thanks! I'm not sure if they belong in a single issue. @lsetiawan could you reply to my unanswered inline comments when you have a chance?

[lsetiawan]

Sorry for the late replies! Just saw those inline comments. This setup is pretty flexible at this moment, I know there are some redundancy mainly with the requirements.txt. I can easily revert back to dynamic reading of that file. I'm simply trying to begin future proof the packaging, and I have no hard fast rule on this at the moment haha 🙈

[emiliom]

This setup is pretty flexible at this moment, I know there are some redundancy mainly with the requirements.txt. I can easily revert back to dynamic reading of that file. I'm simply trying to begin future proof the packaging, and I have no hard fast rule on this at the moment haha 🙈

Thanks. It seems tough trying to balance multiple drivers, such as future proofing vs minimizing duplicated specifications of the same information. I'm not sure what to recommend, though I personally tend to worry more about duplicated information going out of sync and causing confusion.

[leewujung]

I added one comment above.

I personally tend to worry more about duplicated information going out of sync and causing confusion.

I feel the same way. I guess I don't see clear advantage of having everything static in setup.cfg since right now for GH to build the dependency graph requirements.txt still has to exist. The potential out of sync problem seems to make this approach less robust.

Where does the future proof come in here?

[lsetiawan]

I've put back the dynamic generation of requirements. I still kept everything about the extra plotting requirement within the setup.cfg. Now I think it makes a lot of sense to make this optional, and remove the all altogether since there's only one optional dependencies at the moment. Let me know if there's anything else. Otherwise, feel free to merge if all good. Thanks again for both of your inputs 😄

[emiliom]

@lsetiawan thank you for your flexibility as well as for helping us stay on top of current trends. Let's revisit the setup.cfg vs requirements.txt periodically, to assess when the shift to having the dependencies in setup.cfg alone starts to make sense.