Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pytest to deps and CI #156

Merged
merged 14 commits into from
May 6, 2024
Merged

Add pytest to deps and CI #156

merged 14 commits into from
May 6, 2024

Conversation

austinorr
Copy link
Contributor

This PR adds pytest to the optional project dependencies, and should get automated tests passing in CI (partially, there's another PR coming that fixes the packaging/install so that the cli works again).

pyproject.toml Outdated
@@ -14,7 +14,7 @@ description = "Hydrological Simulation Program - Python"
dependencies = [
"cltoolbox",
"numba",
"pandas",
"pandas>=1.5,<2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should refactor all calls to df.append() since it's deprecated. use pandas.concat([df1, df2]) going forward

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did change one df.append() that errored out in my testing in #159.

Why those limits for pandas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because append went from warning to error with V2 and I wanted to punt the refactor. Sounds like we probably caught the same one that is in the test line-of-fire, but there may be many more lurking…

@@ -46,6 +46,11 @@ keywords = [
license = {file = "LICENSE"}
requires-python = ">=3.10"

[project.optional-dependencies]
slim = ["numba", "pandas"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of this library doesn't depend on the cli, so there can be a slim option. In fact, a better approach would be to move the dependency on cltoolbox to an optional dependency tag called cli = ["cltoolbox"].

Copy link
Contributor

@timcera timcera Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote is to keep cltoolbox part of the default dependencies. The CLI is the only way I have run HSP2. A CLI is much easier to use on a cluster or within a parameter estimation run where typically you build out shell scripts or batch files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I don’t mean to propose we remove a cli option. When I use hsp2 on a cluster I use it as a library as part of another program I’ve written that has its own cli. I guess the question here is if you should have to type pip install hsp2[cli] or if I should have to type pip install hsp2[slim]. I don’t truly care either way, as long as there is a slim line available.

Of course we could erase the need for this whole conversation by rewriting the 2 command cli in argparse which I suggest (and volunteer!) to do.

@austinorr
Copy link
Contributor Author

@PaulDudaRESPEC please review. This PR brings test coverage for all code in the project up to 70%. The failing last cli test looks like an additional packaging detail regarding the data files, but tests pass!

@austinorr
Copy link
Contributor Author

@PaulDudaRESPEC also, as of this PR you have the option to adopt a policy of having all PRs be required to pass the test suite (which will grow and change) in order for you to approve and merge. Your review will have to include a check to ensure the PR didn’t somehow make the test easier, haha.

should you adopt such a policy, and I think you should, let’s begin with this PR and hold it until I fix the data package bug. I think I know the fix.

@austinorr austinorr force-pushed the pytest branch 2 times, most recently from ce4e237 to 10fcb7a Compare May 5, 2024 18:59
@austinorr
Copy link
Contributor Author

@PaulDudaRESPEC This PR completes the following:

  • support pandas v1.5-2.0
    • cleared as many warnings and errors as possible on both versions.
    • TODO: in readUCI.py df = df.apply(pd.to_numeric, errors='ignore') 'ignore' is deprecated. implementing fix to use 'raise' or 'coerce' may require some refactoring. I tested 'coerce' but it breaks things.
  • CI test support for pandas 1.5-2.0
  • parallel CI for lint, pytest suite, install + command line invocation.
  • TODO: find much shorter command line test case
  • support python 3.9+, each with CI test for test suite and install.
  • run coverage on recommended python (3.11). This is ready to integrate with coverage trackers like codecov or smokeshow

@austinorr austinorr mentioned this pull request May 6, 2024
@PaulDudaRESPEC PaulDudaRESPEC merged commit 99e8d1e into respec:develop May 6, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants