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

[INF] Simplify environment #1133

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

Zeroto521
Copy link
Member

@Zeroto521 Zeroto521 commented Jul 14, 2022

PR Description

Please describe the changes proposed in the pull request:

  • To keep minimal dependencies.
  • Split environment
    • required: base, work for functions
    • optional: work for others
    • testing: work for testing
    • dev: work for the developer in local
    • doc: work for doc building

this /environment-dev.yaml will be kept for the developer to quickly install pyjanitor environment.
And /ci/envs/* will be created work for CI to test pyjanitor compatibility.

This PR resolves #1127.

PR Checklist

Please ensure that you have done the following:

  1. PR in from a fork off your branch. Do not PR from <your_username>:dev, but rather from <your_username>:<feature-branch_name>.
  1. If you're not on the contributors list, add yourself to AUTHORS.md.
  1. Add a line to CHANGELOG.md under the latest version header (i.e. the one that is "on deck") describing the contribution.
    • Do use some discretion here; if there are multiple PRs that are related, keep them in a single line.

Automatic checks

There will be automatic checks run on the PR. These include:

  • Building a preview of the docs on Netlify
  • Automatically linting the code
  • Making sure the code is documented
  • Making sure that all tests are passed
  • Making sure that code coverage doesn't go down.

Relevant Reviewers

Please tag maintainers to review.

@@ -2,54 +2,43 @@ name: pyjanitor-dev
channels:
- conda-forge
dependencies:
- python=3.9
- python>=3.6
Copy link
Member Author

Choose a reason for hiding this comment

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

Now minimal python version is 3.6
It may don't work in py3.6 with pandas 1.4.1.
it will be updated after setting ci/env multi-envs

Copy link
Collaborator

@samukweku samukweku Jul 14, 2022

Choose a reason for hiding this comment

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

pandas minimum python version is 3.8, I feel we should make it that, or at least 3.7 because of dictionary order

Copy link
Member Author

Choose a reason for hiding this comment

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

py3.6 base on setup.py.
As you said, this is the reason I want to build multi-envs
we could test pyjanitor compatible with python3.6 after ci/env/ was built.

python_requires=">=3.6",

Copy link
Member

Choose a reason for hiding this comment

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

@samukweku @Zeroto521 thanks for doing the PR and reviewing!

I think @samukweku's suggestion makes sense here. With pandas minimum version being 3.8, we should just follow along. I think setup.py should be updated.

- pandas
- pandas-flavor
- multipledispatch
- scipy
Copy link
Member Author

Choose a reason for hiding this comment

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

scipy I thought also could as an optional dependency.
there are only two scripts that use scipy

  • math.py
  • functions/impute.py

- make
# release
- twine
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we could drop twine. we use Github action to build and realse.

Comment on lines 33 to 35
- pre-commit
- pylint
- isort
Copy link
Member Author

Choose a reason for hiding this comment

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

We could drop pylint and isort since we start to use pre-commit.
once we drop these dependencies Makefile also should be updated.

Copy link
Member

Choose a reason for hiding this comment

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

Having pylint and isort binaries in the environment helps with those who use VSCode for code checking. Could we ensure that these stay in the environment while also annotating with comments that they are present for developer convenience?

Copy link
Member Author

Choose a reason for hiding this comment

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

I already keep IDE code style checking packages (black, pylint, isort)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also made a PR #1231 making similar changes. I agree with keeping black/isort, for pylint: we don't enforce it via pre-commit, instead we use flake8 as our main linting tool. So I don't see the need for keeping it around.

@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #1133 (94effb2) into dev (6c43b9d) will decrease coverage by 55.36%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##              dev    #1133       +/-   ##
===========================================
- Coverage   97.34%   41.98%   -55.37%     
===========================================
  Files          77       77               
  Lines        3240     3244        +4     
===========================================
- Hits         3154     1362     -1792     
- Misses         86     1882     +1796     

@Zeroto521
Copy link
Member Author

@samukweku Hi, this problem (test_pivot_longer.py fail) needs you to have a look.
https://github.com/pyjanitor-devs/pyjanitor/runs/7332617267?check_suite_focus=true

@samukweku samukweku added the infrastructure Infrastructure-related issues label Jul 14, 2022
@samukweku
Copy link
Collaborator

@Zeroto521 is it possible to extend this to what users can install? I feel it might help with #826 and #793

@samukweku
Copy link
Collaborator

samukweku commented Jul 14, 2022

@samukweku Hi, this problem (test_pivot_longer.py fail) needs you to have a look.
https://github.com/pyjanitor-devs/pyjanitor/runs/7332617267?check_suite_focus=true

I feel this is triggered because in pandas 3.6, the order of a dictionary is not assured; compared to 3.7 and above, where the order of a dictionary is assured. WIth this, I suggest we make the minimum python version 3.8, as pandas minimum version is 3.8

@Zeroto521
Copy link
Member Author

Zeroto521 commented Jul 14, 2022

@Zeroto521 is it possible to extend this to what users can install? I feel it might help with #826 and #793

I'm in the progress to do this, see #1127.

@Zeroto521
Copy link
Member Author

I feel this is triggered because in pandas 3.6, the order of a dictionary is not assured; compared to 3.7 and above, where the order of a dictionary is assured. WIth this, I suggest we make the minimum python version 3.8, as pandas minimum version is 3.8

conda will install python from a high version to start
if there is an error happening, then install a lower version instead.
And the python version is 3.10.
Does it seem pyjanitor can't work well with py3.10?

 + python                               3.10.5  h582c2e5_0_cpython   conda-forge/linux-64      30MB

https://github.com/pyjanitor-devs/pyjanitor/runs/7332617267?check_suite_focus=true#step:3:834

@Zeroto521
Copy link
Member Author

I finally get some spare time to continue this work. Hope I finished this pr on weekends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Infrastructure-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set a series of complete testing envs
4 participants