-
Notifications
You must be signed in to change notification settings - Fork 76
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
Packaging #256
Packaging #256
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #256 +/- ##
=======================================
Coverage 92.86% 92.87%
=======================================
Files 60 60
Lines 3196 3198 +2
=======================================
+ Hits 2968 2970 +2
Misses 228 228
|
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 for your work in this commit.
At least the remaining debug print should be removed and probably the change to this pytest.mark of test_random_logit
should be reverted again.
Regarding the dropping of py37 support, it's not completely clear to me why python 3.7 support was dropped.
Is it due to the bump up in the torch or tensorflow version?
If it's just because the incoming EOL status for 3.7 I wouldn't mind and support it at least to the point it really breaks.
If it's an issue because some of the required package versions are not available for python 3.7 or the protobuf stuff cannot be fixed easliy in python 3.7, then drop it as it's probably not worth the effort then.
If it's just for keeping the setup.py
logic simpler, I'm ambivalent as I would rather be in favor of getting rid of most of the stuff in setup.py
completely and migrate to a pyproject.toml
. You can have nested extras there too: https://stackoverflow.com/questions/70969565/nest-or-combine-extras-require-for-setuptools-in-setup-cfg
Either way, I think at least some more pressing rationale is needed to completely and finally drop support.
According to https://pypistats.org/packages/quantus there still seem to be some downloads from python3.7 systems, although a very low number percentage-wise.
But just to be clear, I don't see dropping python 3.7 support as very problematic or a deal breaker as the Quantus users will have probably updated to newer versions a long time ago.
… and torch versioning
It looks like the changes duplicate the ones in https://github.com/understandable-machine-intelligence-lab/Quantus/pull/253/files |
Thanks so much @dkrako for many valid/ good points.
I think your point on migrating to Will go ahead with a merge later today, thanks again! |
Hi @aaarrti, this PR is a subset of the PR you have proposed in PR: #253. As I disagree with some of the changes, e.g., dropping 3.7 and then I want to address What I am hoping is to see that PR: #253 will address the improvements related to Also, I did not include your improvement related to running black in CI/CD. So we can also do that in the PR you propose :D Hope this makes sense! |
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.
Ok, let's merge.
Make Quantus more easily maintainable by dropping support for multiple versions and old packages.
requirements_test.txt
.tensorflow
version, fixingprotobuf
incompatibilityrequirements_test.txt
setup.py
file, including adding required and removing unnecessary imports