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 telemetry issues in wheel build and unit tests #4591

Merged
merged 22 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/benchmark_on_push.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

env:
PYBAMM_DISABLE_TELEMETRY: "true"

jobs:
benchmarks:
runs-on: ubuntu-latest
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/periodic_benchmarks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ on:
# workflow manually
workflow_dispatch:

env:
PYBAMM_DISABLE_TELEMETRY: "true"

jobs:
benchmarks:
runs-on: ubuntu-latest
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/publish_pypi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ on:

# Set options available for all jobs that use cibuildwheel
env:
PYBAMM_DISABLE_TELEMETRY: "true"
# Increase pip debugging output, equivalent to `pip -vv`
CIBW_BUILD_VERBOSITY: 2
# Disable build isolation to allow pre-installing build-time dependencies.
Expand Down Expand Up @@ -75,6 +76,7 @@ jobs:
run: pipx run cibuildwheel --output-dir wheelhouse
env:
CIBW_ENVIRONMENT: >
PYBAMM_DISABLE_TELEMETRY="true"
PYBAMM_USE_VCPKG=ON
VCPKG_ROOT_DIR=C:\vcpkg
VCPKG_DEFAULT_TRIPLET=x64-windows-static-md
Expand Down Expand Up @@ -116,6 +118,8 @@ jobs:
- name: Build wheels on Linux
run: pipx run cibuildwheel --output-dir wheelhouse
env:
CIBW_ENVIRONMENT: >
PYBAMM_DISABLE_TELEMETRY="true"
Comment on lines +121 to +122
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell the environment variables were not making it into the CIBW environment. This disables telemetry in the wheel tests

CIBW_ARCHS_LINUX: x86_64
CIBW_BEFORE_ALL_LINUX: >
yum -y install openblas-devel lapack-devel &&
Expand Down Expand Up @@ -242,6 +246,8 @@ jobs:
python scripts/install_KLU_Sundials.py
python -m cibuildwheel --output-dir wheelhouse
env:
CIBW_ENVIRONMENT: >
PYBAMM_DISABLE_TELEMETRY="true"
# 10.13 for Intel (macos-13), 11.0 for Apple Silicon (macos-14 and macos-latest)
MACOSX_DEPLOYMENT_TARGET: ${{ matrix.os == 'macos-14' && '11.0' || '10.13' }}
CIBW_ARCHS_MACOS: auto
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/run_benchmarks_over_history.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ on:
ncommits:
description: "Number of commits to benchmark between commit_start and commit_end"
default: "100"

env:
PYBAMM_DISABLE_TELEMETRY: "true"

jobs:
benchmarks:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/run_periodic_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ on:
- cron: "0 3 * * *"

env:
PYBAMM_DISABLE_TELEMETRY: "true"
FORCE_COLOR: 3
PYBAMM_IDAKLU_EXPR_CASADI: ON
PYBAMM_IDAKLU_EXPR_IREE: ON
Expand Down
5 changes: 1 addition & 4 deletions .github/workflows/test_on_push.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ on:
pull_request:

env:
PYBAMM_DISABLE_TELEMETRY: "true"
FORCE_COLOR: 3
PYBAMM_IDAKLU_EXPR_CASADI: ON
PYBAMM_IDAKLU_EXPR_IREE: ON
Expand Down Expand Up @@ -36,7 +37,6 @@ jobs:
pre-commit run -a

run_unit_integration_and_coverage_tests:
needs: style
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
Expand Down Expand Up @@ -132,7 +132,6 @@ jobs:

# Skips IDAKLU module compilation for speedups, which is already tested in other jobs.
run_doctests:
needs: style
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

runs-on: ubuntu-latest
strategy:
fail-fast: false
Expand Down Expand Up @@ -177,7 +176,6 @@ jobs:
run: python -m nox -s docs

run_example_tests:
needs: style
runs-on: ubuntu-latest
strategy:
fail-fast: false
Expand Down Expand Up @@ -233,7 +231,6 @@ jobs:
run: python -m nox -s examples

run_scripts_tests:
needs: style
runs-on: ubuntu-latest
strategy:
fail-fast: false
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/work_precision_sets.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ on:
types: [published]
workflow_dispatch:

env:
PYBAMM_DISABLE_TELEMETRY: "true"

jobs:
benchmarks_on_release:
if: github.repository_owner == 'pybamm-team'
Expand Down
1 change: 0 additions & 1 deletion noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I could tell, this was not working. It seemed like telemetry was still running in tests.

}
VENV_DIR = Path("./venv").resolve()

Expand Down
32 changes: 21 additions & 11 deletions src/pybamm/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,18 @@
import time


def check_env_opt_out():
return os.getenv("PYBAMM_DISABLE_TELEMETRY", "false").lower() != "false"


def check_opt_out():
opt_out = check_env_opt_out()
config = pybamm.config.read()
if config:
opt_out = opt_out or not config["enable_telemetry"]
return opt_out


def is_running_tests(): # pragma: no cover
"""
Detect if the code is being run as part of a test suite or building docs with Sphinx.
Expand All @@ -21,20 +33,18 @@ def is_running_tests(): # pragma: no cover
):
return True

# Check for GitHub Actions environment variable
if "GITHUB_ACTIONS" in os.environ:
return True

# Check for other common CI environment variables
ci_env_vars = ["CI", "TRAVIS", "CIRCLECI", "JENKINS_URL", "GITLAB_CI"]
ci_env_vars = [
"GITHUB_ACTIONS",
"CI",
"TRAVIS",
"CIRCLECI",
"JENKINS_URL",
"GITLAB_CI",
]
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):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return True

# Check if building docs with Sphinx
if any(mod == "sphinx" or mod.startswith("sphinx.") for mod in sys.modules):
print(
Expand Down Expand Up @@ -110,7 +120,7 @@ def get_input(): # pragma: no cover


def generate():
if is_running_tests():
if is_running_tests() or check_opt_out():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since is_running_tests() does not work in all CI cases, I put the opt-out check here to provide another escape

return

# Check if the config file already exists
Expand Down
47 changes: 27 additions & 20 deletions src/pybamm/telemetry.py
Original file line number Diff line number Diff line change
@@ -1,38 +1,45 @@
from posthog import Posthog
import os
import pybamm
import sys

_posthog = Posthog(
# this is the public, write only API key, so it's ok to include it here
project_api_key="phc_bLZKBW03XjgiRhbWnPsnKPr0iw0z03fA6ZZYjxgW7ej",
host="https://us.i.posthog.com",
)

_posthog.log.setLevel("CRITICAL")
class MockTelemetry:
def __init__(self):
self.disabled = True

@staticmethod
def capture(**kwargs): # pragma: no cover
pass

def disable():
_posthog.disabled = True

if pybamm.config.check_opt_out():
_posthog = MockTelemetry()
else: # pragma: no cover
_posthog = Posthog(
# this is the public, write only API key, so it's ok to include it here
project_api_key="phc_bLZKBW03XjgiRhbWnPsnKPr0iw0z03fA6ZZYjxgW7ej",
host="https://us.i.posthog.com",
)
_posthog.log.setLevel("CRITICAL")


_opt_out = os.getenv("PYBAMM_DISABLE_TELEMETRY", "false").lower()
if _opt_out != "false": # pragma: no cover
disable()
def disable():
_posthog.disabled = True


def capture(event): # pragma: no cover
# don't capture events in automated testing
if pybamm.config.is_running_tests() or _posthog.disabled:
return

properties = {
"python_version": sys.version,
"pybamm_version": pybamm.__version__,
}
if pybamm.config.check_opt_out():
disable()
return

config = pybamm.config.read()
if config:
if config["enable_telemetry"]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is now in the check_out_out() function, so it is redundant here

user_id = config["uuid"]
_posthog.capture(user_id, event, properties=properties)
properties = {
"python_version": sys.version,
"pybamm_version": pybamm.__version__,
}
user_id = config["uuid"]
_posthog.capture(distinct_id=user_id, event=event, properties=properties)
4 changes: 2 additions & 2 deletions tests/shared.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return True


def assert_domain_equal(a, b):
"Check that two domains are equal, ignoring empty domains"
"""Check that two domains are equal, ignoring empty domains"""
a_dict = {k: v for k, v in a.items() if v != []}
b_dict = {k: v for k, v in b.items() if v != []}
assert a_dict == b_dict
18 changes: 15 additions & 3 deletions tests/unit/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,8 @@ def mock_select(*args, **kwargs):
assert "Timeout reached. Defaulting to not enabling telemetry." in captured.out

def test_generate_and_read(self, monkeypatch, tmp_path):
# Mock is_running_tests to return False
monkeypatch.setattr(pybamm.config, "is_running_tests", lambda: False)

# Mock ask_user_opt_in to return True
monkeypatch.setattr(pybamm.config, "check_opt_out", lambda: False)
monkeypatch.setattr(pybamm.config, "ask_user_opt_in", lambda: True)

# Mock telemetry capture
Expand Down Expand Up @@ -155,3 +153,17 @@ def test_read_uuid_from_file_invalid_yaml(self, tmp_path):
config_dict = pybamm.config.read_uuid_from_file(invalid_yaml)

assert config_dict is None

def test_check_opt_out(self, monkeypatch):
monkeypatch.setattr(pybamm.config, "read", lambda: {"enable_telemetry": True})
monkeypatch.setattr(pybamm.config, "check_env_opt_out", lambda: True)
assert pybamm.config.check_opt_out() is True
monkeypatch.setattr(pybamm.config, "read", lambda: {"enable_telemetry": False})
monkeypatch.setattr(pybamm.config, "check_env_opt_out", lambda: True)
assert pybamm.config.check_opt_out() is True
monkeypatch.setattr(pybamm.config, "read", lambda: {"enable_telemetry": True})
monkeypatch.setattr(pybamm.config, "check_env_opt_out", lambda: False)
assert pybamm.config.check_opt_out() is False
monkeypatch.setattr(pybamm.config, "read", lambda: {"enable_telemetry": False})
monkeypatch.setattr(pybamm.config, "check_env_opt_out", lambda: False)
assert pybamm.config.check_opt_out() is True