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

[py312] Skip tests+examples for Tune dependencies that do not support py312 #46645

Closed
wants to merge 17 commits into from
Closed
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
20 changes: 0 additions & 20 deletions .buildkite/ml.rayci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -164,26 +164,6 @@ steps:
--only-tags minimal
depends_on: [ "minbuild-ml", "forge" ]

- label: ":train: ml: tune multinode tests"
tags: tune
instance_type: medium
commands:
- bazel run //ci/ray_ci:build_in_docker -- docker
--platform cpu --image-type ray --canonical-tag multinode
- python ./ci/build/build-multinode-image.py rayproject/ray:multinode rayproject/ray:multinode
- bazel run //ci/ray_ci:test_in_docker -- //python/ray/tune/... ml
--only-tags multinode
--test-env=RAY_HAS_SSH=1
--test-env=RAY_DOCKER_IMAGE=rayproject/ray:multinode
--test-env=RAY_TEMPDIR="/ray-mount"
--test-env=RAY_HOSTDIR="$${RAYCI_CHECKOUT_DIR}"
--test-env=RAY_TESTHOST="rayci.localhost"
depends_on:
- manylinux
- forge
- raycpubase
- mlbuild

- label: ":train: ml: doc tests"
tags:
- train
Expand Down
4 changes: 0 additions & 4 deletions ci/ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,6 @@ compile_pip_dependencies() {
"${WORKSPACE_DIR}/python/requirements/security-requirements.txt" \
"${WORKSPACE_DIR}/python/requirements/anyscale-requirements.txt"

# Remove some pins from upstream dependencies:
# ray, xgboost-ray, lightgbm-ray, tune-sklearn
sed -i "/^ray==/d;/^xgboost-ray==/d;/^lightgbm-ray==/d;/^tune-sklearn==/d" "${WORKSPACE_DIR}/python/$TARGET"

# Delete local installation
sed -i "/@ file/d" "${WORKSPACE_DIR}/python/$TARGET"

Expand Down
1 change: 0 additions & 1 deletion doc/source/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ def __init__(self, version: str):
"https://mvnrepository.com/artifact/*", # working but somehow not with linkcheck
# This should be fixed -- is temporal the successor of cadence? Do the examples need to be updated?
"https://github.com/serverlessworkflow/specification/blob/main/comparisons/comparison-cadence.md",
# TODO(richardliaw): The following probably needs to be fixed in the tune_sklearn package
"https://www.oracle.com/java/technologies/javase-jdk15-downloads.html", # forbidden for client
"https://speakerdeck.com/*", # forbidden for bots
r"https://huggingface.co/*", # seems to be flaky
Expand Down
2 changes: 2 additions & 0 deletions doc/source/tune/examples/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ py_test_run_all_notebooks(
"tune-xgboost.ipynb",
"pbt_transformers.ipynb", # Transformers uses legacy Tune APIs.
"horovod_simple.ipynb", # CI do not have Horovod
"tune-aim.ipynb", # CI does not have `aim`
"bohb_example.ipynb", # CI does not have bohb requirements
],
tags = [
"exclusive",
Expand Down
3 changes: 0 additions & 3 deletions python/ray/air/tests/test_air_usage.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
from ray.tune.callback import Callback
from ray.tune.experiment.experiment import Experiment
from ray.tune.logger import LoggerCallback
from ray.tune.logger.aim import AimLoggerCallback
from ray.tune.utils.callback import DEFAULT_CALLBACK_CLASSES


Expand Down Expand Up @@ -113,7 +112,6 @@ class _CustomCallback(Callback):
wandb.WandbLoggerCallback,
mlflow.MLflowLoggerCallback,
comet.CometLoggerCallback,
AimLoggerCallback,
_CustomLoggerCallback,
_CustomLoggerCallback,
_CustomCallback,
Expand Down Expand Up @@ -152,7 +150,6 @@ def test_tag_setup_mlflow(mock_record, monkeypatch):
"WandbLoggerCallback": 1,
"MLflowLoggerCallback": 1,
"CometLoggerCallback": 1,
"AimLoggerCallback": 1,
"CustomLoggerCallback": 2,
"CustomCallback": 1,
},
Expand Down
8 changes: 0 additions & 8 deletions python/ray/tune/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -159,14 +159,6 @@ py_test(
tags = ["team:ml"],
)

py_test(
name = "test_multinode_sync",
size = "large",
srcs = ["tests/test_multinode_sync.py"],
deps = [":tune_lib"],
tags = ["team:ml", "multinode"],
)

py_test(
name = "test_progress_reporter",
size = "medium",
Expand Down
7 changes: 7 additions & 0 deletions python/ray/tune/examples/bohb_example.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ def load_checkpoint(self, checkpoint_dir):


if __name__ == "__main__":
import sys

import pytest

if sys.version_info >= (3, 12):
Copy link
Contributor

Choose a reason for hiding this comment

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

What version of Python does our CI use? IIRC we used the minimum supported version, but I might be wrong.

Reason I ask is if we don't use 3.12, I'm worried this will get missed in the future, and it might be better to let it explicitly fail and we can make a decision on what to do with the integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@can-anyscale Is CI using py39?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@can-anyscale How often does CI run with py312?

Is it ok if these tests explicitly fail for 312? That way, when py312 becomes the minimum version, we can be aware of what code is broken so it can be fully removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is CI using py39?

CI uses py39 yes

How often does CI run with py312?

You decide; currently we only run tests with py39 versions; historically for other python versions we do a one-fix effort during upgrade but do not run tests continuously

Is it ok if these tests explicitly fail for 312?

We can choose to intentionally NOT run/fix it for python 3.12 now and let it fail explicitly when CI turns py39 to py312

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can choose to intentionally NOT run/fix it for python 3.12 now and let it fail explicitly when CI turns py39 to py312

Explicitly failing on py312 makes sense to me!

Copy link
Collaborator

Choose a reason for hiding this comment

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

sweet, you can just note in https://docs.google.com/spreadsheets/d/1VIpZdrp24CiWkI13EE__REIFlVn0lDiSBLuCN5cyOP8/edit?gid=2025695384#gid=2025695384 that we intentionally ignore the tests for python 3.12 and mark it as green, thankks

pytest.fail("TuneBOHB is not compatible with Python 3.12")

ray.init(num_cpus=8)

config = {
Expand Down
23 changes: 23 additions & 0 deletions python/ray/tune/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,26 @@ def mock_s3_bucket_uri():
logging.getLogger("werkzeug").setLevel(logging.WARNING)
yield s3_uri
logging.getLogger("werkzeug").setLevel(logging.INFO)


# Register the custom mark
def pytest_configure(config):
config.addinivalue_line(
"markers",
"failif(condition, *, reason=...): Fail the test if the condition is true",
)


# Define the custom mark behavior
@pytest.hookimpl(tryfirst=True)
def pytest_runtest_setup(item):
fail_marker = item.get_closest_marker("failif")
if not fail_marker:
return

condition = fail_marker.args[0]
reason = fail_marker.kwargs.get(
"reason", "Test failed due to failif condition being True."
)
if condition:
pytest.fail(reason, pytrace=False)
6 changes: 4 additions & 2 deletions python/ray/tune/tests/test_convergence.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import math
import sys
import unittest

import numpy as np
Expand Down Expand Up @@ -79,6 +80,9 @@ def testConvergenceBayesOpt(self):
assert len(analysis.trials) < 50
assert math.isclose(analysis.best_config["x"], 0, abs_tol=1e-5)

@pytest.mark.skipif(
sys.version_info >= (3, 12), reason="HEBO doesn't support py312"
)
def testConvergenceHEBO(self):
from ray.tune.search.hebo import HEBOSearch

Expand Down Expand Up @@ -137,6 +141,4 @@ def testConvergenceZoopt(self):


if __name__ == "__main__":
import sys

sys.exit(pytest.main(["-v", __file__]))
3 changes: 3 additions & 0 deletions python/ray/tune/tests/test_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
import json
import os
import shutil
import sys
import tempfile
import unittest
from dataclasses import dataclass
from pathlib import Path
from typing import Optional

import numpy as np
import pytest

import ray
from ray.air.constants import (
Expand Down Expand Up @@ -295,6 +297,7 @@ def testBadTBX(self):
assert "INFO" in cm.output[0]


@pytest.mark.skipif(sys.version_info >= (3, 12), reason="Aim doesn't support py312")
class AimLoggerSuite(unittest.TestCase):
"""Test Aim integration."""

Expand Down
210 changes: 0 additions & 210 deletions python/ray/tune/tests/test_multinode_sync.py

This file was deleted.

Loading
Loading