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

Fix LFS errors for #20 #21

Closed
wants to merge 23 commits into from
Closed

Fix LFS errors for #20 #21

wants to merge 23 commits into from

Conversation

StanczakDominik
Copy link
Member

@StanczakDominik StanczakDominik commented May 1, 2020

This is a temporary draft PR so I can experiment with CI and LFS
to fix #20's test failures while not polluting that commit tree.

Before merging:

namurphy and others added 15 commits April 30, 2020 15:43
These tests will not work since they are in files that were directly
copied over from the NEI-modeling/NEI repository.
The class is adapted from the class EigenData2 which is in the
eigenvaluetable.py file that will be deleted soon.  The tests are
adapted from the test_eigenvaluetable.py class that will also be
deleted soon.
These were copied over from the NEI-modeling/NEI repository.
Co-authored-by: Chengcai Shen <[email protected]>
Co-authored-by: Marcus Dupont <[email protected]>
This is done as per the docs:
https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/pipeline-options-for-git?view=azure-devops#checkout-files-from-lfs

It seems azure pipelines doesn't, by default, checkout LFS files.
I realized this when I had issues cloning the repo including the LFS
file.
@pep8speaks
Copy link

pep8speaks commented May 1, 2020

Hello @StanczakDominik! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 67:27: E203 whitespace before ':'

Line 17:1: E303 too many blank lines (3)
Line 37:1: E302 expected 2 blank lines, found 3
Line 406:89: E501 line too long (104 > 88 characters)
Line 991:89: E501 line too long (91 > 88 characters)
Line 1120:61: E203 whitespace before ':'
Line 1167:53: E203 whitespace before ':'
Line 1173:9: E731 do not assign a lambda expression, use a def

Comment last updated at 2020-05-01 10:14:39 UTC

@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #21 into master will decrease coverage by 18.82%.
The diff coverage is 81.15%.

Impacted file tree graph

@@             Coverage Diff              @@
##            master      #21       +/-   ##
============================================
- Coverage   100.00%   81.17%   -18.83%     
============================================
  Files            2        4        +2     
  Lines            4      712      +708     
============================================
+ Hits             4      578      +574     
- Misses           0      134      +134     
Impacted Files Coverage Δ
plasmapy_nei/nei/nei.py 79.43% <79.39%> (-20.57%) ⬇️
plasmapy_nei/eigen/eigenclass.py 86.11% <86.11%> (ø)
plasmapy_nei/eigen/__init__.py 100.00% <100.00%> (ø)
plasmapy_nei/nei/__init__.py 100.00% <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 1137689...828d435. Read the comment docs.

@StanczakDominik
Copy link
Member Author

StanczakDominik commented May 1, 2020

Gotcha! As per the docs at https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/pipeline-options-for-git?view=azure-devops#checkout-files-from-lfs, it's the lack of this line in the OpenAstro run-tox-env.yaml template that's causing the LFS file not to get pulled during the checkout stage:

https://github.com/PlasmaPy/PlasmaPy-NEI/pull/21/files#diff-fa9e59b6afe6a6ecdc0d1ee19d3befaeR59

I'm opening up a PR there.

StanczakDominik added a commit to StanczakDominik/azure-pipelines-templates that referenced this pull request May 1, 2020
We ran into the issue of Azure not pulling git-lfs files during
PlasmaPy/PlasmaPy-NEI#21

As it turns out, adding `lfs: true` to the `checkout` step
as in this change:

PlasmaPy/PlasmaPy-NEI@d3360c3

allows Azure to pull LFS files.

This change was guided by Azure docs over at
https://docs.microsoft.com/en-us/azure/devops/pipelines/repos/pipeline-options-for-git?view=azure-devops#checkout-files-from-lfs
@StanczakDominik
Copy link
Member Author

StanczakDominik commented May 1, 2020

Okay @namurphy, that should do the trick. It's probably best if you merge this branch into your PR before doing any further work there.

@StanczakDominik
Copy link
Member Author

For some reason the windows tox test is taking a good while longer. That's strange...

@StanczakDominik
Copy link
Member Author

the wheel and sdist tests are failing, the low-ish (I think...) size of the built package as per https://dev.azure.com/plasmapy/PlasmaPy-NEI/_build/results?buildId=778&view=logs&j=f7656e4a-aab1-5409-7a28-08f0d640eba6&t=a9542dd9-4ae8-5071-2817-2d1dda94f840&l=64 seems to imply the h5 file is not getting included in the sdist itself... A package_data issue?

@StanczakDominik
Copy link
Member Author

I've reverted the changes as lfs: true is now upstream :)

@namurphy namurphy deleted the eigen-fix-h5 branch August 14, 2020 20:43
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