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.

Sign up for GitHub

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

Jump to bottom

Fix pyedr data packaging and deploy on push #54

Merged
merged 7 commits into from
Nov 7, 2022
Merged

Fix pyedr data packaging and deploy on push #54

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7bdb440
Fix pyedr data packaging and deploy on push
IAlibay Jul 15, 2022
9ac53f3
Update ChangeLog
IAlibay Jul 18, 2022
3714793
Merge branch 'master' into fix-data-packaging
orbeckst Sep 1, 2022
6384be3
Merge branch 'master' into fix-data-packaging
IAlibay Nov 6, 2022
4292891
more specific manifest
IAlibay Nov 6, 2022
4cf6580
Update changelog
IAlibay Nov 6, 2022
526d9ae
Merge branch 'fix-data-packaging' of github.com:MDAnalysis/panedr int…
IAlibay Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/workflows/deploy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@ name: Build and upload to PyPI

on:
push:
branches:
- "master"
tags:
- "*"
release:
Expand Down
11 changes: 10 additions & 1 deletion ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,16 @@ The rules for this file:

------------------------------------------------------------------------------

15/06/2022 jbarnoud, BFedder, orbeckst, hmacdope, IAlibay
18/07/2022 IAlibay

* 0.6.1

Fixes

* Adds pyedr/tests/data to manifest for packaging.


15/07/2022 jbarnoud, BFedder, orbeckst, hmacdope, IAlibay

* 0.6.0

Expand Down
1 change: 1 addition & 0 deletions pyedr/MANIFEST.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
include ../README.rst ../LICENSE.txt
include ../ChangeLog ../AUTHORS
include setup.py setup.cfg
include pyedr/tests/data/*
Copy link
Member

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.

Copy link
Member Author

@IAlibay IAlibay Jul 18, 2022 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member

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???

Copy link
Member Author

@IAlibay IAlibay Jul 18, 2022 •
edited
Loading

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

Copy link
Member Author

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 and cat.xvg much smaller given they are ~ 14M each (compared to ~ 100kB for the other files), @jbarnoud would need to weigh in on this here.

Copy link
Collaborator

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.

Copy link
Member

@orbeckst orbeckst Jul 18, 2022 •
edited
Loading

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.

Copy link
Collaborator

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