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

RF: Cleanup Makefile to bring parity with CI, remove tox + nose references #1105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jacobdr
Copy link
Contributor

@jacobdr jacobdr commented May 7, 2022

Closes #1104

  • Align on venv as virtual environment convention
  • Update makefile test target to re-use same script used in CI to orchestrate testing
  • Remove refererences to tox and nose
  • Fix dev-requirements.txt and doc-requirements.txt to pin to versions that are non-breaking

…test target to re-use same script used in CI to orchestrate testing. Remove refererences to tox and nose
@pep8speaks
Copy link

pep8speaks commented May 7, 2022

Hello @jacobdr, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 nibabel.

Comment last updated at 2022-05-07 19:05:52 UTC

@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #1105 (ad2cc68) into master (7cfaebf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1105   +/-   ##
=======================================
  Coverage   92.29%   92.29%           
=======================================
  Files         100      100           
  Lines       12247    12247           
  Branches     2393     2393           
=======================================
  Hits        11303    11303           
  Misses        621      621           
  Partials      323      323           

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 7cfaebf...ad2cc68. Read the comment docs.

…ure flake8 (closer aligns to pep8speaks bot behavior)
@@ -252,11 +252,13 @@ sdist-venv: clean
rm -rf dist venv
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if this target is still being used anywhere -- so I kind of hacked it to install the dev dependencies to get it to pass on my local

@@ -1,6 +1,8 @@
# Requirements for building docs
-r requirements.txt
sphinx<3
numpydoc
jinja2<3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to pin these to get version compatibility to work. In the future might be better to use a tool like poetry so that we can generate a lockfile and not have to deal with this sort of not-fun dependency pinning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed this to get my local to pass but it seems like CI might not like it.... I'll try reverting to see if if this is what is breaking the pre-release CI jobs

*test*
*sphinx*
nibabel/externals/*
*/__init__.py
# temporary -- should be removed in another PR
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need a follow-up PR to clean up the remaining issues. I added these as ignores so that local flake8 passes. These were being covered up by the configuration of pep8speaks only looking at the diff

source virtenv/bin/activate
elif [ -e virtenv/Scripts/activate ]; then
source virtenv/Scripts/activate
if [ -e venv/bin/activate ]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was made so that we are consistent in local dev and remote dev with the virtualenv



ut-%: build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this as I did not see it being used anywhere. I can revert this if a maintainer feels it should be brought back

@jacobdr
Copy link
Contributor Author

jacobdr commented May 8, 2022

@effigies all the pre-release tests failed for the same reason -- a string representation in a doctest for netcdf_file. I don't immediately see how my changes would have introduced this. Looking at this now, but any thoughts as to root cause?

@jacobdr
Copy link
Contributor Author

jacobdr commented May 8, 2022

Trying to narrow differences... since mmap is coming from the standard lib I suspected a change in Python versions and how repr(mmap_obj) or str(mmap_obj) is displayed... Its possible, though still smells to me so needs a sanity check.

As evidence, though, comparing against a prior (chose #1096 as the most recent unmerged green build I saw....)

Python version comparison

Final note: the only pre-release job that passes is the one that does not have the PRE_PIP_FLAGS environment variable set....

Comment on lines +95 to +97
.PHONY: test-style
test-style:
export CHECK_TYPE=style; ./tools/ci/check.sh
Copy link
Member

Choose a reason for hiding this comment

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

I would rather put make style in check.sh than call check.sh from here. And likewise with other tests.

Suggested change
.PHONY: test-style
test-style:
export CHECK_TYPE=style; ./tools/ci/check.sh
.PHONY: style
style:
flake8

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.

Update Makefile to remove reference to nosetests
3 participants