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.

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

Separate out densepose install for contributing instructions #179

Merged
merged 8 commits into from
Apr 13, 2022

Conversation

ejm714
Copy link
Collaborator

@ejm714 ejm714 commented Mar 22, 2022

requirements-dev.txt no longer installs the densepose extra since DensePose requires torch to be installed first. The appropriate install and test commands for DensePose contributions are now laid out on the contributing page.

Bonus fixes

  • update installation instructions to use v4 as brew will now install v5 by default
  • skip densepose tests by default so that make tests runs with the default dev install. the verbose pytest will show that these tests are skipped

@ejm714 ejm714 requested a review from pjbull March 22, 2022 20:31
@github-actions
Copy link
Contributor

github-actions bot commented Mar 22, 2022

@ejm714
Copy link
Collaborator Author

ejm714 commented Mar 23, 2022

Okay @pjbull this is ready for review. Test failures are unrelated though I am looking into those

@ejm714
Copy link
Collaborator Author

ejm714 commented Mar 23, 2022

Update: this PR now fixes the failing the test. The issue was that some of the test assets don't have that many frames since we heavily downsampled them -- if they have fewer than 10, then selecting the 10th frame index in the single frame test will fail. I added a sort in this commit to ensure we're always using the same video for that test which has enough frames to be a valid test case.

@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #179 (071735a) into master (61b64b2) will increase coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #179     +/-   ##
========================================
+ Coverage    85.0%   85.2%   +0.1%     
========================================
  Files          30      30             
  Lines        1851    1851             
========================================
+ Hits         1575    1578      +3     
+ Misses        276     273      -3     
Impacted Files Coverage Δ
zamba/data/video.py 80.9% <0.0%> (+1.2%) ⬆️

Copy link
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

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

LGTM except one question about the densepose tests

docs/docs/contribute/index.md Show resolved Hide resolved
@@ -164,7 +164,7 @@ def test_train_save_dir_overwrite(
"train_configuration.yaml",
"test_metrics.json",
"val_metrics.json",
"epoch=0-step=10.ckpt",
"epoch=0-step=11.ckpt",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Caused by recent change in 1.6.0 (released 2022-03-29)

The filename of checkpoints created with ModelCheckpoint(filename='{step}') is different compared to previous versions. A checkpoint saved after 1 step will be named step=1.ckpt instead of step=0.ckpt (Lightning-AI/pytorch-lightning#11805)

@ejm714 ejm714 merged commit 3de1b8a into master Apr 13, 2022
@ejm714 ejm714 deleted the contributing-install branch April 13, 2022 21:25
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