Fix telemetry issues in wheel build and unit tests #4591
Conversation
| CIBW_ENVIRONMENT: > | ||
| PYBAMM_DISABLE_TELEMETRY="true" |
There was a problem hiding this comment.
As far as I can tell the environment variables were not making it into the CIBW environment. This disables telemetry in the wheel tests
| @@ -132,7 +131,6 @@ jobs: | |||
|
|
|||
| # Skips IDAKLU module compilation for speedups, which is already tested in other jobs. | |||
| run_doctests: | |||
| needs: style | |||
There was a problem hiding this comment.
If style fails then PR is updated with the pre-commit fixes and the old jobs get canceled. This just speeds up the start of the jobs a bit
| @@ -49,7 +49,6 @@ def set_iree_state(): | |||
| "IREE_INDEX_URL": os.getenv( | |||
| "IREE_INDEX_URL", "https://iree.dev/pip-release-links.html" | |||
| ), | |||
| "PYBAMM_DISABLE_TELEMETRY": "true", | |||
There was a problem hiding this comment.
As far as I could tell, this was not working. It seemed like telemetry was still running in tests.
| if any(var in os.environ for var in ci_env_vars): | ||
| return True | ||
|
|
||
| # Check for common test runner names in command-line arguments | ||
| test_runners = ["pytest", "unittest", "nose", "trial", "nox", "tox"] | ||
| if any(runner in arg.lower() for arg in sys.argv for runner in test_runners): |
There was a problem hiding this comment.
This did not seem to work on Mac or Windows. I was seeing nox and pytest commands go right past this. I tested it by moving it to the top of the file and putting an assert below it.
| @@ -110,7 +116,7 @@ def get_input(): # pragma: no cover | |||
|
|
|||
|
|
|||
| def generate(): | |||
| if is_running_tests(): | |||
| if is_running_tests() or check_opt_out(): | |||
There was a problem hiding this comment.
Since is_running_tests() does not work in all CI cases, I put the opt-out check here to provide another escape
|
|
||
| config = pybamm.config.read() | ||
| if config: | ||
| if config["enable_telemetry"]: |
There was a problem hiding this comment.
This check is now in the check_out_out() function, so it is redundant here
| @@ -336,12 +336,12 @@ def no_internet_connection(): | |||
| conn = socket.create_connection((host, 80), 2) | |||
| conn.close() | |||
| return False | |||
| except socket.gaierror: | |||
| except (socket.gaierror, TimeoutError): | |||
There was a problem hiding this comment.
Sometimes I get timeouts from the no_internet_connection() function. This makes that also count as "not connection", but since I was actively connected to the internet, I don't think this function is working as expected.
kratman
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4591 +/- ##
===========================================
- Coverage 99.26% 99.25% -0.02%
===========================================
Files 302 302
Lines 22889 22906 +17
===========================================
+ Hits 22721 22735 +14
- Misses 168 171 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
Coverage is easy to fix. I will do that before I mark it ready for review |
kratman
requested review from
martinjrobins,
Saransh-cpp,
agriyakhetarpal,
arjxn-py and
a team
as code owners
|
Wheel build passed, this should be ready for the release |
kratman
added
the
release blocker
kratman
requested review from
valentinsulzer,
MarcBerliner,
rtimms and
aabills
Description
Fixes the wheel build and updates the telemetry code to make sure it does not run during tests
Fixes #4588
Type of change
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: