-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
@csadorf @bdice @tommy-waltmann The I think we should pin the version in the |
@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:
|
@bdice should I go ahead and change the version numbers in both requirements file and Also, I think we need to uncheck the required column for the job |
I found out that the minimum tqdm version we require is 4.43.0! The tests are failing for anything below it. |
The tests will pass now, I think this is ready! @csadorf @bdice @tommy-waltmann please review whenever you get time! Thanks. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
.circleci/config.yml
Outdated
jinja2==2.10 \ | ||
cloudpickle==1.1.1 \ | ||
deprecation==2 \ | ||
tqdm==4.43.0 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now done!
Good find! It's a fix that was in tqdm PR 893. https://tqdm.github.io/releases/#v4430-2020-02-19 |
I have created a requirements file called |
There was a problem hiding this 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.
Description
Jinja2
versions older than2.10.0
.tqdm
versions older than4.43.0
.Motivation and Context
Replicates glotzerlab/signac#474 for
flow
.Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist:
If necessary: