Skip to content

Conversation

@jeromekelleher
Copy link
Member

Support loading/dumping tree sequences to real files rather than just paths.

See #565 for background.

Not for merging - this temporarily pulls in kastore, but needs rebasing when we bring in kastore 2.0 properly as noted in the first commit.

@jeromekelleher jeromekelleher marked this pull request as draft May 10, 2020 16:45
@jeromekelleher
Copy link
Member Author

Need to fix tskit-dev/kastore#121 before taking this up again.

@jeromekelleher jeromekelleher force-pushed the dumpload-fileobj branch 2 times, most recently from 6702127 to 3499210 Compare May 11, 2020 12:56
@jeromekelleher jeromekelleher requested a review from benjeffery May 11, 2020 12:57
@jeromekelleher
Copy link
Member Author

I think I'm going to leave it here with just the C changes required for working with files, and push the Python changes needed to a later release. Managing arbitrary file handles isn't on anyones critical path for Python, I think, so we don't need to push it.

@grahamgower, would you mind taking a look over this please? Are you OK with this just being in the C API for now, or do you want file streams in Python also for your apps?

Should be ready to merge after we've pulling in kastore C API 2.0.0 and a rebase/tidyup.

@grahamgower
Copy link
Member

This is excellent, thanks @jeromekelleher! The C API is the bit I wanted to use, yes. Of course, I'm sure I'll use the Python interface too when it exists, but I don't have any immediate plans for that.

@jeromekelleher
Copy link
Member Author

Thanks @grahamgower - I'll queue the Python version up for release 3.1 so. It'd be nice to just do it now, but there's too much going on.

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #599 into master will increase coverage by 0.02%.
The diff coverage is 84.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   87.64%   87.66%   +0.02%     
==========================================
  Files          23       23              
  Lines       17431    17501      +70     
  Branches     3434     3450      +16     
==========================================
+ Hits        15277    15342      +65     
- Misses       1055     1058       +3     
- Partials     1099     1101       +2     
Flag Coverage Δ
#c_tests 88.89% <84.00%> (+0.01%) ⬆️
#python_c_tests 91.11% <100.00%> (+<0.01%) ⬆️
#python_tests 98.95% <100.00%> (ø)
Impacted Files Coverage Δ
c/tskit/tables.c 79.13% <82.14%> (+0.02%) ⬆️
c/tskit/trees.c 90.77% <84.61%> (+0.11%) ⬆️
c/tskit/core.c 91.39% <100.00%> (+0.78%) ⬆️
python/_tskitmodule.c 83.91% <100.00%> (+0.01%) ⬆️
python/tskit/cli.py 96.29% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5b38e4...0199fbb. Read the comment docs.

@jeromekelleher jeromekelleher marked this pull request as ready for review May 25, 2020 17:48
@jeromekelleher
Copy link
Member Author

This is ready for review, whenever suits @benjeffery (no hurry)

@jeromekelleher
Copy link
Member Author

The coverage is a little low on this one, but I think I've covered everything that can be hit. In particular, the kastore_openf in tsk_table_collection_dumpf can't be errored, as it doesn't actually do anything with the file object until it writes to the file.

Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Just the one typo.
Are we planning on bringing this functionality to the cli? e.g. tskit info < stream.trees

- Fix issue in which TSK_NO_INIT was not being observed for load_tables.
@jeromekelleher
Copy link
Member Author

LGTM, Just the one typo.

Great, let's get this one queued up for merging then.

Are we planning on bringing this functionality to the cli? e.g. tskit info < stream.trees

Yes, but let's put it off until 0.3.1. There's enough going on for 0.3 for now, and this was really to get the change into the C API.

@mergify mergify bot merged commit 781f8ea into tskit-dev:master Jun 1, 2020
@jeromekelleher jeromekelleher deleted the dumpload-fileobj branch June 1, 2020 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants