Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix pyedr data packaging and deploy on push #54
Fix pyedr data packaging and deploy on push #54
Changes from 1 commit
7bdb440
9ac53f3
3714793
6384be3
4292891
4cf6580
526d9ae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Be more specific so that backup files are not included??? Not sure if it’s worth the effort but wanted to mention 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.
I'm not sure I understand what you're trying convey here - essentially the issue we have is that the datafiles needed to run the tests, using say
pytest --pyargs pyedr.tests
aren't being included in the package. This means that the tests won't run.That being said, if we don't want to include the tests files here that's fine too, we just won't tell folks that they can test locally outside of a source installation. In some ways I would much prefer that because the tarball for
pyedr
with the test files is ~ 15 MB, compared to 11.8 kB without.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 just meant that instead of
*
use*.xvg
or whatever more specific pattern includes the files.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 see the point of not including test files with the package — seems overkill to make it 15 MB.
If we exclude the files then the tests should produce a message that state how one can get them.
I also don’t know how down-stream packagers like it if the tests cannot be run easily.
Is there a way to include the files in the tar file but exclude from wheel and conda???
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.
Ah I see, in that case my vote would be towards it's not worth it. We shouldn't have backup files in there to begin with, and specific including file types means remembering to update the manifest every time we add a new file type (which can easily be forgotten about).
If we want to have the test files here, we should just bundle them all - that being said, I'm increasingly convinced that we shouldn't bundle them here, 14MB as a tarball means ~ 50 MB unpacked, that seems like too much of a burden, especially for what is likely to be an MDA core dependency.
edit: sorry didn't see your second message
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.
Oops hadn't seen your second message come in @orbeckst - :/ it might be possible to make the tarball & wheels different, but then it's hard to make conda pick up something else that isn't the release or the pypi tarball (if that makes sense).
Essentially it gets rather messy.
One option here is to make
cat.edr
andcat.xvg
much smaller given they are ~ 14M each (compared to ~ 100kB for the other files), @jbarnoud would need to weigh in on this here.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.
If I recall correctly, I left the whole tests out initially to have the less cumbersome experience for the users. If pyedr is meant to be even leaner, it makes sense to have the same approach. I also like packages being light, I am currently rewriting some of my code in rust in part because shipping gigabytes of dependences was becoming impractical.
I am not a huge fan of shipping the tests without the data though, as the data is an integral part of the tests. I should still be able to live with 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.
I pretty much agree with everything (edit:) @IAlibay said. (I hadn’t seen @jbarnoud ‘s comment while writing)
I’d be pragmatic for right now, though, and err on the side of “correctness” and completeness and merge the pr as is to create a package that includes everything you need to check that it’s working as advertised.
Then we have a basis to work from and we can decide how best to shrink the package footprint: if we can make the files smaller and keep the package under, say, 1 MB, then that would be a good way. Otherwise , removing the test files is probably the way to go.
I’ll leave it to you and @jbarnoud if you want to merge the pr or continue discussion.
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 tested using much smaller files in #55 and coverage seems unaffected, so if we want to reduce the test size I can get that PR ready for merging