-
Notifications
You must be signed in to change notification settings - Fork 60
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 initial Python unit/performance tests for TIFF loader module #62
Conversation
4d20d78
to
7e972c6
Compare
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.
Thanks Gigon! 😄
Had a couple suggestions about CI changes that I think we can now drop as well as one question
CuPy 9 is now default so RC release is not needed.
__PROJECT__ needs to be cuCIM.
When calculating the maximal number of tiles (y-axis) overlapped with the given patch, patch size souldn't be larger than the original image size. Consider the fact during the calculation.
aac3f7a
to
c1dc1c5
Compare
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.
Approving ops-codeowner
file changes
d8cbbe4
to
5c1f329
Compare
OpenSlide's result can be different among environments due to differences in JPEG dequantization of the JPEG libraries so remove such logic.
@jakirkham now it is building. Could you please help review 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.
Hi Gigon, this looks mostly good to me. I left just some minor comments.
The style of passing the image parameters as a string with :
separator seems a little un-Pythonic, but this is only for test code, not part of the public API, so I don't have a problem with it.
I have not yet tried running this code, but can try that on the 5th or 6th.
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.
Thank you @grlee77 for the all valuable review! Please help review the updated code.
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.
Thanks, the test dependency installation looks much cleaner now in the run
script.
It would be nice to also update the developer docs to mention how the tests should be run (in CONTRIBUTING.md?)
@grlee77 Addressed your comments. The following commits are added: |
cca3585
to
5148e28
Compare
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 reviewed the recent commits, making only a few small suggestions
Thank you for adding the clean
and clean_cache
options to the run script!
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.
Thanks Gigon! 😄
This looks pretty good. Also like the addition of the clean
command 🙂
Had a couple minor questions about the environment file
@@ -16,8 +16,12 @@ dependencies: | |||
- scipy | |||
- python=3.8 | |||
- cudatoolkit=11.0 | |||
- cudatoolkit-dev=11.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.
Why is this needed?
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.
In Conda package, cudatoolkit package doesn't include nvcc
binary, but nvcc is needed to build the project so added it. Does it make sense? Any other solution to do that? :)
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.
Yeah it doesn't. We are working on a longer term solution ( conda-forge/cudatoolkit-feedstock#62 )
However this package was added for historical reasons to help users run the CUDA installer. I don't think we should be using it
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.
@jakirkham for now, what would be a good solution until the long-term solution is available?
Currently, without installing cudatoolkit-dev, it would try to find system's nvcc if available which cannot be isolated in the environment. Do you think we can keep the change(cudatoolkit-dev
) until the long-term solution is available?
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.
Is it ok if we just rely on the system one for now? Or are there other issues with that?
Should add that others may just be using our prebuilt packages. So think this is more a question of what works for us
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.
Is it ok if we just rely on the system one for now? Or are there other issues with that?
I thought env.yml
is for setting up 'development environment' to build cuCIM locally from the source code and it is not for the runtime environment (it already has more than needed for just running cuCIM. e.g., yasm/compilers/cmake/flake8 etc).
I don't think all users (developer who want to contribute here) have the same nvcc version needed (in this case, CUDA 11.0) in the system so I think cudatoolkit-dev
with the same version of cudatoolkit
is needed.
Maybe we need to separate out env.yml
-- one for dev, one for runtime.
Hi @jakirkham, can this PR be merged or do I need to address others? |
@gpucibot merge |
Thanks Gigon! 😄 |
The main change is for addressing parts of #38 and #61.
This also has the following minor changes:
And build/doc related changes
Current test folder structure looks like below: