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.

Sign up for GitHub

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

Jump to bottom

Remove coveragepy-lcov dependency during CI. #1382

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Remove coveragepy-lcov dependency during CI. #1382

merged 2 commits into from
Aug 9, 2023

Conversation

ntouran
Copy link
Member

@ntouran ntouran commented Aug 9, 2023 •
edited by john-science
Loading

What is the change?

coveragepy-lcov is incompatible with coverage 7.0.0 and in fact is totally unnecessary now that pytest can write lcov format files directly. This switches pytest to write lcov directly. Now coverage should work in CI on python 3.11.

Fixes #1377

I have not fully tested this but tested the components. Will need to let CI run to see if it fully works I think.

Why is the change being made?

We want coverage on python 3.11

Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any important changes.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@ntouran
Remove coveragepy-lcov dependency during CI.
5eeb949 
coveragepy-lcov is incompatible with coverage 7.0.0 and
in fact is totally unnecessary now that pytest can write
lcov format files directly. This switches pytest
to write lcov directly. Now coverage should work in CI
on python 3.11

Fixes #1377
@john-science john-science added the documentation Improvements or additions to documentation label Aug 9, 2023
@john-science
Copy link
Member

I LOVE removing dependencies. Awesome.

And I note that the "coverage" step on GH Actions has already passed for this PR.

You rock, man. Thanks!

opotowsky
opotowsky requested changes Aug 9, 2023
View reviewed changes
Copy link
Member

@opotowsky opotowsky left a comment

Choose a reason for hiding this comment

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

Two things that I can't comment inline:

  1. In .github/workflows/coverage.yaml Can you update python 3.10 to 3.11?
  2. In setup.py, you'll note a silly little python-version-specific coverage version update I made to get the first py311 PR merged. I think given this GH action cleanup we can revert that update, and just remove the coverage version pin altogether.

@ntouran
Copy link
Member Author

ntouran commented Aug 9, 2023

ok done. Part of me wonders if coverage belongs in the install_requires section at all, since it's not used at runtime, but I'm happy to clean out those pins for now.

@john-science
Copy link
Member

ok done. Part of me wonders if coverage belongs in the install_requires section at all, since it's not used at runtime, but I'm happy to clean out those pins for now.

I turns out coverage is used in the base Case class:

armi/armi/cases/case.py

Line 41 in 06bf1f1

import coverage

armi/armi/cases/case.py

Line 66 in 06bf1f1

COVERAGE_RESULTS_FILE = "coverage_results.cov"

armi/armi/cases/case.py

Line 353 in 06bf1f1

cov = self._startCoverage()

And when I suggested we could pull that out because it's not used, I believe @jakehader lobbied quite hard that this was used and shouldn't be removed. (I might be mis-remembering WHO lobbied so hard, but someone.)

opotowsky
opotowsky approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@opotowsky opotowsky left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this!

@opotowsky
Copy link
Member

ok done. Part of me wonders if coverage belongs in the install_requires section at all, since it's not used at runtime, but I'm happy to clean out those pins for now.

I turns out coverage is used in the base Case class:

armi/armi/cases/case.py

Line 41 in 06bf1f1

import coverage

armi/armi/cases/case.py

Line 66 in 06bf1f1

COVERAGE_RESULTS_FILE = "coverage_results.cov"

armi/armi/cases/case.py

Line 353 in 06bf1f1

cov = self._startCoverage()

And when I suggested we could pull that out because it's not used, I believe @jakehader lobbied quite hard that this was used and shouldn't be removed. (I might be mis-remembering WHO lobbied so hard, but someone.)

Actually this might have a viable use case and it's been on my ToDo backburner for a few months. So I'd also lobby to keep it until we know it's not useful!

@ntouran ntouran merged commit 18f3200 into main Aug 9, 2023
@ntouran ntouran deleted the ntouran/1377-coverage branch August 9, 2023 21:40
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Aug 24, 2023
@drewj-usnctech
Merge remote-tracking branch 'terrapower/main' into drewj-usnctech/gr…
df4bfa4 
…id-refactor-pre-1278

* terrapower/main:
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  Add Python 3.11 to GH Actions (terrapower#1341)
  Removing global variable from runLog.py (terrapower#1370)
  Moving the First-Time Contributors guide to the docs (terrapower#1368)
drewj-usnctech added a commit to drewj-usnctech/armi that referenced this pull request Sep 6, 2023
@drewj-usnctech
Merge remote-tracking branch 'terrapower/main' into drewj-usnctech/ma…
6c1ab49 
…t-mod-empty-validation

* terrapower/main: (23 commits)
  Allowing users to set powerDensity instead of power (terrapower#1395)
  Fix assemNum parameter for uniform mesh reactor. (terrapower#1398)
  Update to black v22.6.0 (terrapower#1396)
  Fixing gallery example for new working Grids (terrapower#1394)
  Refactor armi.reactor.grids to wider submodule; Grids as ABC (terrapower#1373)
  Make SFP a child of reactor. (terrapower#1349)
  Move max assembly number out of global scope (terrapower#1383)
  Ensuring we throw an error on bad YAML files (terrapower#1358)
  Removing unused settings (terrapower#1393)
  Fixing some miscellaneous TODOs (terrapower#1392)
  Removing global plugin flags (terrapower#1388)
  Update reactivity coefficient parameters (terrapower#1355)
  Fixing ruff formatting of doc gallery examples (terrapower#1389)
  Fixing broken link (terrapower#1390)
  Removing unreachable code block (terrapower#1391)
  Removing unnecessary f-strings (terrapower#1387)
  Updating documentation dependencies for Python 3.11 (terrapower#1378)
  Adding a little code coverage to the CLIs (terrapower#1371)
  Remove coveragepy-lcov dependency during CI. (terrapower#1382)
  Reconnect logger after disconnecting in test. (terrapower#1381)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opotowsky opotowsky opotowsky approved these changes

@john-science john-science john-science approved these changes

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Find a way to run coverage on python 3.11
3 participants
@ntouran @john-science @opotowsky