Move requirements-dev.txt dependencies to environment.yml #812

seisman · 2021-01-28T23:13:51Z

Description of proposed changes

Delete the 'requirements-dev.txt' file, and move two of the dependencies (make and codecov) to the conda 'environment.yml' file. This allows the "Install GMT and required dependencies" step in '.github/workflows/ci_tests.yaml' to be simplified.

All these packages/dependencies can be installed via pip, so the comments in
these two files are not accurate.

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If adding new functionality, add an example to docstrings or tutorials.

Notes

You can write /format in the first line of a comment to lint the code automatically

All these packages can be installed via `pip`, so the comments in these two files are not accurate.

weiji14

Since we're on this, can we think about consolidating PyGMT's dependency files:

requirements.txt
requirements-dev.txt
environment.yml

This mess seems to be a hangover from the Fatiando a Terra days (#187, sorry Leo). We could probably move the requirements*.txt files to pyproject.toml and keep the environment.yml for conda only.

seisman · 2021-01-29T13:43:56Z

Since we're on this, can we think about consolidating PyGMT's dependency files:

requirements.txt

requirements-dev.txt

environment.yml

This mess seems to be a hangover from the Fatiando a Terra days (#187, sorry Leo). We could probably move the requirements*.txt files to pyproject.toml and keep the environment.yml for conda only.

Do you mean poetry? It doesn't support dynamic versioning from setuptools_scm, see #667 (comment).

weiji14 · 2021-01-29T21:33:16Z

Do you mean poetry? It doesn't support dynamic versioning from setuptools_scm, see #667 (comment).

Yes I'm thinking of poetry. The dynamic versioning is still defined in setup.py at this line

pygmt/setup.py

Line 38 in 4ed63f8

SETUP_REQUIRES = ["setuptools_scm"]

right? I still think we can move the dependency list to the pyproject.toml file (but keep the setup.py intact).

Let's keep this PR on hold for now, I'll try and see if I can move requirements*.txt to pyproject.toml after sorting out other higher priority feature PRs.

willschlitzer · 2021-01-29T23:17:32Z

@seisman and @weiji14 , you are definitely more experienced developers than I am, but I feel like it is common practice in Python repos to have a requirements.txt. While I understand wanting to consolidate files for the sake of organization, I think requirements.txt and requirements-dev.txt are pretty standard files, and are recognizable to someone looking to get involved in the project and needing to install the necessary packages.

weiji14 · 2021-01-29T23:46:09Z

@seisman and @weiji14 , you are definitely more experienced developers than I am, but I feel like it is common practice in Python repos to have a requirements.txt. While I understand wanting to consolidate files for the sake of organization, I think requirements.txt and requirements-dev.txt are pretty standard files, and are recognizable to someone looking to get involved in the project and needing to install the necessary packages.

Traditionally yes, requirements.txt is a 'standard', but have a read about the blog post at https://snarky.ca/what-the-heck-is-pyproject-toml/ which mentions PEP518 (see also the original discussion at #667). There's actually a way to pip install --use-pep517, and using pyproject.toml will likely be more common in the future (though it will take time).

As a compromise, I'm happy to keep the requirements.txt file, but move requirements-dev.txt to pyproject.toml. And/or we can improve the page at https://www.pygmt.org/dev/install.html#installing-pygmt to make it easier for new developers to get started.

willschlitzer · 2021-02-12T08:04:04Z

@GenericMappingTools/python Do you have any inputs on whether we are going to move to requirements-dev.txt to pyproject.toml? I read the blog post that @weiji14 left and I don't see the advantages of switching to use a .toml file, but I'm also not too familiar with the work involved in Python packaging. I'm fine with whatever we go with, but think that our contributing guide should include how to use pyproject.toml if we do go with that option.

weiji14 · 2021-02-12T08:46:25Z

@GenericMappingTools/python Do you have any inputs on whether we are going to move to requirements-dev.txt to pyproject.toml? I read the blog post that @weiji14 left and I don't see the advantages of switching to use a .toml file, but I'm also not too familiar with the work involved in Python packaging. I'm fine with whatever we go with, but think that our contributing guide should include how to use pyproject.toml if we do go with that option.

Let's keep requirements.txt (the compromise I mentioned in #812 (comment)), but delete requirement-dev.txt and move it to environment.yml (see 5612a68). The pyproject.toml shift will require some more work and documentation and can happen at a later date.

Delete the requirements-dev.txt file, and update the ci_test.yml to install GMT dependencies directly from environment.yml.

weiji14 · 2021-02-15T00:27:11Z

So I tried moving the dependencies from requirements-dev.txt to environment.yml in commits 17613f2 and f822ab3, and now the CI tests can't find the GMT library 😅 Feel free to revert those changes, or I'll do more troubleshooting/edits after release v0.3.0.

seisman · 2021-02-15T00:33:21Z

So I tried moving the dependencies from requirements-dev.txt to environment.yml in commits 17613f2 and f822ab3, and now the CI tests can't find the GMT library 😅 Feel free to revert those changes, or I'll do more troubleshooting/edits after release v0.3.0.

From the build log, it seems that GMT is installed in the pygmt environment, but PyGMT is installed in the test environment.

#
# To activate this environment, use
#
#     $ conda activate pygmt
#
# To deactivate an active environment, use
#
#     $ conda deactivate

and

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/share/miniconda3/envs/test/lib/python3.7/site-packages/pygmt/__init__.py", line 39, in <module>
    _begin()
  File "/usr/share/miniconda3/envs/test/lib/python3.7/site-packages/pygmt/session_management.py", line 16, in begin
    with Session() as lib:
  File "/usr/share/miniconda3/envs/test/lib/python3.7/site-packages/pygmt/clib/session.py", line 183, in __enter__
    self.create("pygmt-session")
  File "/usr/share/miniconda3/envs/test/lib/python3.7/site-packages/pygmt/clib/session.py", line 333, in create
    restype=ctp.c_void_p,
  File "/usr/share/miniconda3/envs/test/lib/python3.7/site-packages/pygmt/clib/session.py", line 282, in get_libgmt_func
    self._libgmt = load_libgmt()
  File "/usr/share/miniconda3/envs/test/lib/python3.7/site-packages/pygmt/clib/loading.py", line 48, in load_libgmt
    "Error loading the GMT shared library "
pygmt.exceptions.GMTCLibNotFoundError: Error loading the GMT shared library libgmt.so.
 libgmt.so: cannot open shared object file: No such file or directory.
make: *** [Makefile:33: test] Error 1

weiji14 · 2021-02-15T01:13:01Z

From the build log, it seems that GMT is installed in the pygmt environment, but PyGMT is installed in the test environment.

Ok, fixed in 6958959. Just needed to use conda env update --name test --file environment.yml.

seisman · 2021-02-15T04:42:41Z

From the build log, it seems that GMT is installed in the pygmt environment, but PyGMT is installed in the test environment.

Ok, fixed in 6958959. Just needed to use conda env update --name test --file environment.yml.

Not sure if it works, but I feel it's better to change the setup-miniconda default environment name to pygmt, and remove --name pygmt. See https://github.com/conda-incubator/setup-miniconda#use-a-different-environment-name-or-path

.github/workflows/ci_tests.yaml

seisman · 2021-02-17T03:21:33Z

environment.yml

@@ -3,7 +3,6 @@ channels:
    - conda-forge
    - defaults
 dependencies:
-    - python=3.8


What about adding a few comment lines to group the dependencies into:

required dependencies

dev dependencies

CI-specific dependencies (codecov, and make?)

Ok. But should we consider make a CI dependency, or a dev dependency? I feel as though make here might help on Windows, see #914 (comment).

Perhaps just do required and dev dependencies (see the Suggested Changes I made below). codecov is a tiny 16.4kb wheel. make is 6.0MB on Windows but could be considered a dev dependency too.

Looks good.

.github/workflows/ci_tests.yaml

environment.yml

Divide dependency list into `pygmt` required and development dependencies. Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

seisman · 2021-02-17T21:18:27Z

Looks good to me, but I can't approve it because I opened this PR.

weiji14

Oops, almost forgot you opened this. Good to merge then!

…pingTools#812) * Move dev dependency specification to environment.yml * Use `conda env update` instead of `conda install` * Let setup miniconda auto activate pygmt conda environment * Add section headers to environment.yml file Co-authored-by: Dongdong Tian <seisman.info@gmail.com> Co-authored-by: Will Schlitzer <schlitzer90@gmail.com> Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>

Fix the comments in requirements.txt and requirements-dev.txt

3a5fa7d

All these packages can be installed via `pip`, so the comments in these two files are not accurate.

seisman added maintenance Boring but important stuff for the core devs skip-changelog Skip adding Pull Request to changelog labels Jan 28, 2021

seisman added this to the 0.3.0 milestone Jan 28, 2021

seisman requested review from a team January 28, 2021 23:14

Merge branch 'master' into fix-requirements-comments

0f63f1a

vercel bot temporarily deployed to Preview January 29, 2021 01:51 Inactive

weiji14 reviewed Jan 29, 2021

View reviewed changes

Merge branch 'master' into fix-requirements-comments

b593ea3

vercel bot temporarily deployed to Preview January 29, 2021 13:44 Inactive

seisman marked this pull request as draft January 30, 2021 05:19

Merge branch 'master' into fix-requirements-comments

c75fc35

vercel bot temporarily deployed to Preview February 12, 2021 08:07 Inactive

seisman modified the milestones: 0.3.0, 0.4.0 Feb 14, 2021

weiji14 added 2 commits February 15, 2021 12:43

Merge branch 'master' into fix-requirements-comments

dfd2bda

Move dev dependency specification to environment.yml

17613f2

Delete the requirements-dev.txt file, and update the ci_test.yml to install GMT dependencies directly from environment.yml.

vercel bot temporarily deployed to Preview February 14, 2021 23:44 Inactive

Use conda env update instead of conda install

f822ab3

vercel bot temporarily deployed to Preview February 14, 2021 23:55 Inactive

Let conda update the 'test' environment, not 'pygmt'

6958959

vercel bot temporarily deployed to Preview February 15, 2021 01:03 Inactive

seisman commented Feb 15, 2021

View reviewed changes

.github/workflows/ci_tests.yaml Show resolved Hide resolved

Let setup miniconda auto activate pygmt conda environment

ca703a3

vercel bot temporarily deployed to Preview February 15, 2021 05:23 Inactive

weiji14 force-pushed the master branch from 1730198 to 65b099a Compare February 15, 2021 12:06

weiji14 changed the title ~~Fix the comments in requirements.txt and requirements-dev.txt~~ Move requirements-dev.txt dependencies to environment.yml Feb 17, 2021

seisman commented Feb 17, 2021

View reviewed changes

weiji14 reviewed Feb 17, 2021

View reviewed changes

environment.yml Show resolved Hide resolved

environment.yml Show resolved Hide resolved

Add section headers to environment.yml file

4364312

Divide dependency list into `pygmt` required and development dependencies. Co-authored-by: Dongdong Tian <seisman.info@gmail.com>

vercel bot temporarily deployed to Preview February 17, 2021 21:02 Inactive

Merge branch 'master' into fix-requirements-comments

94a91fe

weiji14 marked this pull request as ready for review February 17, 2021 21:03

vercel bot temporarily deployed to Preview February 17, 2021 21:10 Inactive

seisman modified the milestones: 0.4.0, 0.3.1 Feb 17, 2021

weiji14 approved these changes Feb 17, 2021

View reviewed changes

seisman removed the skip-changelog Skip adding Pull Request to changelog label Feb 17, 2021

seisman merged commit 64702cd into master Feb 17, 2021

seisman deleted the fix-requirements-comments branch February 17, 2021 21:33

weiji14 mentioned this pull request Mar 13, 2021

Initialize data version control for managing test images #1036

Merged

5 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move requirements-dev.txt dependencies to environment.yml #812

Move requirements-dev.txt dependencies to environment.yml #812

seisman commented Jan 28, 2021 •

edited by weiji14

Loading

weiji14 left a comment

seisman commented Jan 29, 2021

weiji14 commented Jan 29, 2021

willschlitzer commented Jan 29, 2021

weiji14 commented Jan 29, 2021

willschlitzer commented Feb 12, 2021

weiji14 commented Feb 12, 2021

weiji14 commented Feb 15, 2021 •

edited

Loading

seisman commented Feb 15, 2021

weiji14 commented Feb 15, 2021

seisman commented Feb 15, 2021

seisman Feb 17, 2021

weiji14 Feb 17, 2021

weiji14 Feb 17, 2021

seisman Feb 17, 2021

seisman commented Feb 17, 2021

weiji14 left a comment

Move requirements-dev.txt dependencies to environment.yml #812

Move requirements-dev.txt dependencies to environment.yml #812

Conversation

seisman commented Jan 28, 2021 • edited by weiji14 Loading

weiji14 left a comment

Choose a reason for hiding this comment

seisman commented Jan 29, 2021

weiji14 commented Jan 29, 2021

willschlitzer commented Jan 29, 2021

weiji14 commented Jan 29, 2021

willschlitzer commented Feb 12, 2021

weiji14 commented Feb 12, 2021

weiji14 commented Feb 15, 2021 • edited Loading

seisman commented Feb 15, 2021

weiji14 commented Feb 15, 2021

seisman commented Feb 15, 2021

seisman Feb 17, 2021

Choose a reason for hiding this comment

weiji14 Feb 17, 2021

Choose a reason for hiding this comment

weiji14 Feb 17, 2021

Choose a reason for hiding this comment

seisman Feb 17, 2021

Choose a reason for hiding this comment

seisman commented Feb 17, 2021

weiji14 left a comment

Choose a reason for hiding this comment

seisman commented Jan 28, 2021 •

edited by weiji14

Loading

weiji14 commented Feb 15, 2021 •

edited

Loading