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

Update dependencies #860

Merged
merged 6 commits into from
Aug 28, 2020
Merged

Update dependencies #860

merged 6 commits into from
Aug 28, 2020

Conversation

ali-ramadhan
Copy link
Member

@ali-ramadhan ali-ramadhan commented Aug 17, 2020

Some regular maintenance

@ali-ramadhan
Copy link
Member Author

Hmmm, so it looks like tests take longer now so Travis CI and GitLab CI will more often than not time out and fail 😞

Maybe it's time to split up the tests into 3-4 smaller jobs?

@navidcy
Copy link
Collaborator

navidcy commented Aug 19, 2020

I don't understand why you have the Manifest.toml? With proper use of the [compat] section in Project.toml there is no need to have the Manifest.toml in the package (and thus no need to keep updating it). Am I missing something?

@glwagner
Copy link
Member

glwagner commented Aug 19, 2020

@navidcy that is only true in theory I think when package versions can be inferred from the dependency tree. In reality we need to maintain the precise versions of packages that are used... ?

@ali-ramadhan
Copy link
Member Author

@navidcy I think you're right that with proper [compat] bounds, a Manifest.toml is unnecessary. But I think that would mean that as developers we would have to be meticulous about updating lower and upper bounds of dependencies. In particular, if a bug is introduced in a dependency then it could cause our master branch to fail (unless we had very strict upper [compat] bounds and updated them regularly).

I guess I see strict [compat] bounds as saying that "our package is guaranteed to work as long as your dependencies lie between these versions" (might take a lot of testing to ensure all version combinations are fine), while just using a Manifest.toml with loose [compat] bounds is saying "our package is guaranteed to work with the versions prescribed in the Manifest.toml".

I like the Manifest.toml approach as it gives us more control: we only update dependencies when we're ready and deal with any new issues one-by-one before merging. And it lets us play around with upgrading/downgrading the version on different dependencies, e.g. CUDA.jl, without worrying about [compat] bounds.

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #860 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #860   +/-   ##
=======================================
  Coverage   70.90%   70.90%           
=======================================
  Files         187      187           
  Lines        5180     5180           
=======================================
  Hits         3673     3673           
  Misses       1507     1507           

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 46608d7...19390e0. Read the comment docs.

@ali-ramadhan
Copy link
Member Author

Hmmm looks like it was TEST_GROUP=integration that went from taking ~20 minutes to ~32 minutes, enough to cause timeouts. We can live with this for now I suppose, can look into it in the future.

@ali-ramadhan ali-ramadhan merged commit 810a3a1 into master Aug 28, 2020
@ali-ramadhan ali-ramadhan deleted the ar/update-deps branch August 28, 2020 00:02
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.

3 participants