-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Build and test the documentation in Azure Pipelines #3854
Conversation
|
dba768c
to
d673e8b
Compare
Alright! The documentation testing seems to work! I had to implement several workarounds:
Otherwise, the PR is ready for review. |
@@ -93,17 +93,17 @@ class BRIEF(DescriptorExtractor): | |||
>>> extractor.extract(square2, keypoints2) | |||
>>> descriptors2 = extractor.descriptors | |||
>>> matches = match_descriptors(descriptors1, descriptors2) | |||
>>> matches | |||
>>> matches # doctest: +SKIP |
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.
why these tests are skipped?
# Extract all possible features to be able to select the most salient. | ||
feature_coord, feature_type = \ | ||
|
||
if __name__ == '__main__': |
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'm a big -1 on this. This really confuses new users, who become confused since they don't knkow what __main__
means. why do we need to do this?
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.
@hmaarrfk I'm still trying to figure out a better solution. The issue is described here - #3854 (comment), specifically, in the StackOverflow post.
In short, on Windows multiprocessing imports the full contents of the file when spawning extra processes. This leads to starting an infinite number of processes.
So, basically, the multiprocessing code has to be wrapped somehow.
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.
sigh, ok.
@@ -64,18 +64,18 @@ class BRIEF(DescriptorExtractor): | |||
>>> import numpy as np | |||
>>> square1 = np.zeros((8, 8), dtype=np.int32) | |||
>>> square1[2:6, 2:6] = 1 | |||
>>> square1 | |||
>>> square1 # doctest: +SKIP |
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 think you should get linux setup, and run tehse doctests on linux. Linux will be able to handle the doctests and all.
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.
@Borda I think conftest.py
file is not being discovered by pytest on Windows. In that file we force numpy to use legacy printoptions. Therefore, the builds expect the newer notation (that is numerically equivalent, but visually different).
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.
@hmaarrfk on Azure you mean? I'm not sure if anyone from the core team is willing to switch from Travis to Azure at the moment. Of course, it shouldn't be an issue to have a Linux instance running, but I really want to have Windows build with the full coverage.
Unfortunately, googling "pytest, windows, conftest" doesn't yield any info. I'm going to check if forcing conftest could be done with the env.variables...
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.
We could also just test the doctests on numpy 1.14 and higher
or just bunp the minimum reqs all the way up to 1.14 ;)
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.
@soupault, it might be worthwhile too post an issue on pytest's github page. I couldn't figure this out either, it could very well be a bug on windows.
I understand that you want full test coverage on windows, but I would argue that having the doctests be readable is more important. I've often seen my comments copied verbatim in other's code. I wouldn't watn to pollute their dropbox with # doctest: +SKIP ;)
The work is continued in #3873. |
Description
Implement documentation building and testing in Azure Pipelines, similarly to https://github.com/scikit-image/scikit-image/blob/master/tools/travis/script.sh.
For reviewers
later.
__init__.py
.doc/release/release_dev.rst
.@meeseeksdev backport to v0.14.x