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 oldest & minimal CI configurations. #436

Merged
merged 25 commits into from
Jan 24, 2021
Merged

Add oldest & minimal CI configurations. #436

merged 25 commits into from
Jan 24, 2021

Conversation

kidrahahjo
Copy link
Collaborator

@kidrahahjo kidrahahjo commented Jan 22, 2021

Description

  • Adds "oldest" CI configuration. Python 3.6 may no longer be supported by the latest versions of our testing dependencies.
  • Re-organizes all the requirements files into a subdirectory called requirements.
  • Add minimum version 1.1.1 for cloudpickle in the requirements file. (Tests fail on older versions)
  • Drops support for Jinja2 versions older than 2.10.0.
  • Drops support for tqdm versions older than 4.43.0.

Motivation and Context

Replicates glotzerlab/signac#474 for flow.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog.

Sorry, something went wrong.

@kidrahahjo kidrahahjo requested review from a team as code owners January 22, 2021 14:36
@kidrahahjo kidrahahjo requested review from csadorf and tommy-waltmann and removed request for a team January 22, 2021 14:36
@kidrahahjo kidrahahjo marked this pull request as draft January 22, 2021 15:24
@kidrahahjo
Copy link
Collaborator Author

@csadorf @bdice @tommy-waltmann The contrib submodule in tqdm was added 4.40.0 (Check release notes)

I think we should pin the version in the requirement files and setup.py accordingly.

@kidrahahjo kidrahahjo marked this pull request as ready for review January 22, 2021 15:41
@bdice
Copy link
Member

bdice commented Jan 22, 2021

@kidrahahjo On the topic of tqdm, we need 4.42.0 or newer to have the wrappers around concurrent futures. See this comment in #417:

Uses a tqdm wrapper around concurrent futures. Note: if we choose to use this approach (which is a lot less total code), we will need to bump our tqdm version requirement to tqdm>=4.42.0.

https://github.com/tqdm/tqdm/releases/tag/v4.42.0

@kidrahahjo
Copy link
Collaborator Author

kidrahahjo commented Jan 22, 2021

@kidrahahjo On the topic of tqdm, we need 4.42.0 or newer to have the wrappers around concurrent futures. See this comment in #417:

Uses a tqdm wrapper around concurrent futures. Note: if we choose to use this approach (which is a lot less total code), we will need to bump our tqdm version requirement to tqdm>=4.42.0.

https://github.com/tqdm/tqdm/releases/tag/v4.42.0

@bdice should I go ahead and change the version numbers in both requirements file and setup.py to 4.42.0 for tqdm?

Also, I think we need to uncheck the required column for the job linux-python-36 since we renamed it to linux-python-36-oldest

@kidrahahjo
Copy link
Collaborator Author

@csadorf @bdice I have Python 3.8.5 running on my machine and it seems like we don't support Jinja2 versions older than 2.10.

@bdice
Copy link
Member

bdice commented Jan 22, 2021

@csadorf @bdice I have Python 3.8.5 running on my machine and it seems like we don't support Jinja2 versions older than 2.10.

That’s correct. There are template filters we require from 2.10.

@kidrahahjo
Copy link
Collaborator Author

I found out that the minimum tqdm version we require is 4.43.0! The tests are failing for anything below it.

@kidrahahjo
Copy link
Collaborator Author

The tests will pass now, I think this is ready! @csadorf @bdice @tommy-waltmann please review whenever you get time! Thanks.

@codecov
Copy link

codecov bot commented Jan 22, 2021

Codecov Report

Merging #436 (0ded6e9) into master (8a07953) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   70.45%   70.55%   +0.10%     
==========================================
  Files          29       29              
  Lines        2999     2999              
  Branches      555      555              
==========================================
+ Hits         2113     2116       +3     
+ Misses        738      734       -4     
- Partials      148      149       +1     
Impacted Files Coverage Δ
flow/project.py 73.86% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a07953...0ded6e9. Read the comment docs.

Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

I just have one idea that might make things look a little cleaner.

Comment on lines 68 to 71
jinja2==2.10 \
cloudpickle==1.1.1 \
deprecation==2 \
tqdm==4.43.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be best to just put these dependencies in another file, similar to requirements.txt, and requriements-test.txt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a nice idea, and I think making a file called requirements-oldest.txt should do the work. But we'd then have to configure depandabot accordingly because I think it modifies all the files having the requirements keyword in it. (Which would make the implementation more complicated).

@tommy-waltmann let me know what you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how re-configuring dependabot would work, I think @bdice or @csadorf would be better able to comment on that.

Copy link
Member

Choose a reason for hiding this comment

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

Right, as @kidrahahjo noted, the challenge is that we don't want dependabot to update the "oldest" dependency file. We might be able to add an exception but I couldn't figure out how to do that from the dependabot documentation. If someone else can identify a solution for that, I would definitely prefer that to hard-coding the "oldest" requirements in the CI config.

Copy link
Contributor

Choose a reason for hiding this comment

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

If dependabot just looks for files that have the requirements substring in them, we could just have a file with a different name.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that dependabot looks in all files named *requirements*.txt and also requirements/*.txt, so we could name it .circleci/ci-oldest-reqs.txt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now done!

@bdice
Copy link
Member

bdice commented Jan 22, 2021

I found out that the minimum tqdm version we require is 4.43.0! The tests are failing for anything below it.

Good find! It's a fix that was in tqdm PR 893. https://tqdm.github.io/releases/#v4430-2020-02-19

@kidrahahjo
Copy link
Collaborator Author

I have created a requirements file called .circleci/ci-oldest-reqs.txt for testing the oldest versions. In that file, I also pinned the signac version to 1.0.0.

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Nice work, @kidrahahjo! I have a few suggested changes. I'm going to work on applying those changes now.

@bdice bdice self-requested a review January 23, 2021 20:48
@bdice bdice merged commit f455d74 into master Jan 24, 2021
@bdice bdice deleted the ci/oldest branch January 24, 2021 01:37
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