Lightning-Universe / lightning-transformers

Flexible components pairing 🤗 Transformers with :zap: Pytorch Lightning
https://lightning-transformers.readthedocs.io
Apache License 2.0
607 stars 77 forks source link

Fix protobuf version #286

Closed turian closed 1 year ago

turian commented 1 year ago

See https://github.com/Lightning-AI/lightning-transformers/pull/284#issuecomment-1243050284

This is based upon @rohitgr7 's awesome https://github.com/Lightning-AI/lightning-transformers/pull/284

codecov[bot] commented 1 year ago

Codecov Report

Merging #286 (05ab2b0) into master (11f2056) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 05ab2b0 differs from pull request most recent head 44130e9. Consider uploading reports for the commit 44130e9 to get more accurate results

@@          Coverage Diff          @@
##           master   #286   +/-   ##
=====================================
  Coverage      74%    74%           
=====================================
  Files          63     63           
  Lines        1474   1474           
=====================================
  Hits         1088   1088           
  Misses        386    386           
turian commented 1 year ago

@rohitgr7 @carmocca @Borda Any idea why my CI is failing? Is it the skipped tests or the warnings? I didn't grep for any errors.

Even if @rohitgr7 's https://github.com/Lightning-AI/lightning-transformers/pull/284 gets merged, I still would like to understand how to placate CI in case I do future PRs.

carmocca commented 1 year ago

Looks like the tests pass, but there's an error either in the coverage computation or on pytest teardown.

I would start by removing coverage and see if the result is different: https://github.com/Lightning-AI/lightning-transformers/actions/runs/3033498716/workflow#L76-L77

turian commented 1 year ago

Looks like the tests pass, but there's an error either in the coverage computation or on pytest teardown.

I would start by removing coverage and see if the result is different: https://github.com/Lightning-AI/lightning-transformers/actions/runs/3033498716/workflow#L76-L77

Thanks for the pointer. But where in the CI logs are you looking specifically, besides the final non-descript 'Error'?

carmocca commented 1 year ago

besides the final non-descript 'Error'?

Just here, there's nothing else. This is why I suspect the command does not finish properly. As I pointed out it's coverage calling pytest. So even if pytest passes, coverage could fail (my hypothesis)

To check it, do

-coverage run --source lightning_transformers -m py.test lightning_transformers tests -v --junitxml=junit/test-results-${{ runner.os }}-${{ matrix.python-version }}-${{ matrix.requires }}.xml
+python -m py.test lightning_transformers tests -v
-coverage XML
Borda commented 1 year ago

@turian the needed proval is GH policy notning that we came up :)

Borda commented 1 year ago

@turian, I guess this is not needed anymore after #284 was merged, correct? :rabbit:

turian commented 1 year ago

@turian, I guess this is not needed anymore after #284 was merged, correct? 🐰

Correct