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

HARMONY-388: Better consistency across Python repos #13

Merged
merged 11 commits into from Jun 8, 2021
Merged

Conversation

vinnyinverso
Copy link
Contributor

What follows are the acceptance criteria and what I found or did to address them. (I also spoke with Chris prior to submitting this PR.)

  • Consistent python environment setup and use across the 3 repos
    • For all but the harmony-service example, the READMEs were suggesting to use some combination of pyenv, pyenv-virtualenv, or venv. I tried to update the READMEs so that it was clear to the reader that all of these were acceptable tools.
  • Either build scripts or make across the 3 repos
    • All of the regular (non-Docker service) Python module/lib repos (harmony-service-lib, harmony-py) use make files. All of the Docker/service-oriented repos (harmony-netcdf-to-zarr, harmony-service-example) use build scripts because they seem more conducive to the scripting logic needed to satisfy some of the more complex build logic. This seemed like a reasonably consistent solution (makefiles for pure libraries and build scripts for docker services).
  • Consistent way of installing deps across the 3 repos
    • All except one repo are using pip for installing dependencies in a fairly consistent manner, with one requirements file for dev, and one for core requirements. The only repo not following this pattern is the harmony-service-example, which uses conda. However, conda is a recommended way of installing one of the main dependencies of this repo (gdal) whereas pip is not (and is difficult to use in this instance from my experience). The use of conda, is also still compatible with the aforementioned recommended Python version manager and virtual env tools.
  • Consistent linting across the 3 repos
    • All repos (except for harmony-service-example) use flake8. We could add flake8 to harmony-service-example if desirable. I'm guessing it wasn't added due to the simplicity of the example.
  • Consistent unit testing framework and execution across the 3 repos
    • I believe Patrick said there was another ticket addressing this.
  • Consistent CI/CD across the 3 repos
    • Much of this seems to be adequately addressed by the recent transition to GitHub and related work.
  • Update READMEs across the 3 repos to document the consistent tools, packages, and configuration for setting up a dev environment; dev environment does not presume editor
    • I updated 3 READMEs to give more consistent suggestions on virtual environment setup. None of the dev environment setups assume the user has a specific editor.

@vinnyinverso
Copy link
Contributor Author

Had to update faker to fix an issue with the github actions test failures in newer versions of Python: joke2k/faker#1439

README.md Outdated
@@ -104,8 +104,8 @@ OPTIONAL -- Use with CAUTION:
## Development Setup

Prerequisites:
- Python 3.7+, ideally installed via a virtual environment such as `pyenv`
- A local copy of the code
- Python 3.7+, ideally installed via a virtual environment. We recommend using [pyenv](https://github.com/pyenv/pyenv) for managing multiple Python installations with different versions, in conjunction with a tool like [pyenv-virtualenv](https://github.com/pyenv/pyenv-virtualenv) or [Python's venv module](https://docs.python.org/3.7/library/venv.html) for creating an isolated virtual environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a recommendation - what about we just recommend venv built-in with Python rather than pyenv-virtualenv as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't do that is because in a similar discussion a month or so ago, Patrick seemed to allude that we should not be too opinionated on which virtual environment tool is used, as long as there's a virtual environment. So here I was just trying to make the reader aware that they can use either. Also, in the description of this ticket, there was a line about trying to make things "more similar to harmony-py" -- which offers both venv and pyenv-virtualenv as viable tools. Although this alone doesn't make this repo identical to harmony-py, it seemed like a step in that direction, without being too disruptive. I'm ok with either route and think you have a good point -- just explaining my thought process.

Copy link
Member

Choose a reason for hiding this comment

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

I was mostly suggesting that to avoid complicating the end-user documentation, i.e. not mentioning or requiring one as a setup step. It feels akin to telling people which editor to use (though I think we may also do that in other docs...)

@vinnyinverso vinnyinverso changed the title HARMONY-388: Make additional recommendation on Python environment setup HARMONY-388: Better consistency across Python repos May 28, 2021
README.md Outdated
@@ -104,12 +104,12 @@ OPTIONAL -- Use with CAUTION:
## Development Setup

Prerequisites:
- Python 3.7+, ideally installed via a virtual environment such as `pyenv`
- Python 3.7+, ideally installed via a [virtual environment](https://github.com/nasa/harmony-service-example/blob/main/ENVHELP.md)
Copy link
Member

Choose a reason for hiding this comment

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

Small nit (that I'm not even sure I'm right about): I'm on the fence about whether we should link across repos like this, but if we do, we should probably link in the direction of our dependency arrows. harmony-service-example depends on this repo, so the ENVHELP.md should go in this repo with harmony-service-example pointing to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does feel kinda weird. I think I'll remove it.

@vinnyinverso vinnyinverso merged commit b7730b0 into main Jun 8, 2021
@vinnyinverso vinnyinverso deleted the HARMONY-388 branch June 8, 2021 13:38
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.

None yet

3 participants