Skip to content

Conversation

@benjeffery
Copy link
Member

Stacked on #2157
Adds restricted_copy to the low-level python. I think I've covered all the lifecycle stuff here, but a close review would be appriciated.

@benjeffery benjeffery changed the title Variant copy python Variant copy for lowlevel python Mar 31, 2022
@benjeffery
Copy link
Member Author

Note to self - run the stress test for leaks.

@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #2169 (a2c01fa) into main (fecad52) will decrease coverage by 0.01%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2169      +/-   ##
==========================================
- Coverage   93.29%   93.28%   -0.02%     
==========================================
  Files          27       27              
  Lines       26039    26059      +20     
  Branches     1163     1163              
==========================================
+ Hits        24294    24308      +14     
- Misses       1715     1721       +6     
  Partials       30       30              
Flag Coverage Δ
c-tests 92.20% <ø> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 72.27% <71.42%> (-0.01%) ⬇️
python-tests 98.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/_tskitmodule.c 90.94% <71.42%> (-0.07%) ⬇️

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 fecad52...a2c01fa. Read the comment docs.

@benjeffery
Copy link
Member Author

Looks like the linter has broken deps, will investigate.

@benjeffery
Copy link
Member Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Apr 1, 2022

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/2)
Auto-merging python/_tskitmodule.c
CONFLICT (content): Merge conflict in python/_tskitmodule.c
Auto-merging python/tests/test_lowlevel.py
CONFLICT (content): Merge conflict in python/tests/test_lowlevel.py
error: could not apply 28a4394... Add initial lowlevel Variant class
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 28a4394... Add initial lowlevel Variant class

err-code: F4A65

@jeromekelleher
Copy link
Member

I can take a look at this once it's rebased @benjeffery

@benjeffery benjeffery force-pushed the variant-copy-python branch from cd42909 to 4bb7f39 Compare April 4, 2022 09:32
@jeromekelleher
Copy link
Member

Looks right to me, but I'd run some serious stress tests on this sort of thing to reassure myself as it's very subtle.

@benjeffery benjeffery force-pushed the variant-copy-python branch 2 times, most recently from db3eb8f to 6f35de2 Compare April 4, 2022 12:34
@benjeffery
Copy link
Member Author

Looks right to me, but I'd run some serious stress tests on this sort of thing to reassure myself as it's very subtle.

Stress test is fine, max iter is at 100 after 1000 runs. I've also gone over the code again to try and satisfy myself.

@jeromekelleher
Copy link
Member

I'd usually run in a loop in a standalone script as well, as the stress script is quite slow and noisy when checking specific changes like this. Problems usually show up very quickly.

@benjeffery
Copy link
Member Author

I modified the stress script to only run the Variant tests. I'll also run a tight loop before merging though.

@mergify mergify bot merged commit 48c16df into tskit-dev:main Apr 4, 2022
@benjeffery benjeffery deleted the variant-copy-python branch April 4, 2022 23:40
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.

2 participants