Skip to content

Commit 67c7dac

Browse files
author
Bhanu Teja Goshikonda
committed
sanity: apply Sirut feedback — trim EFA comment, dedupe conftest fixtures
Trims EFA install-NGC comment per review. Removes image_uri and default_region fixture overrides in TF sagemaker conftest — parent test/conftest.py already declares them. Renames default_region -> region in test files to match parent. Workflow updated to pass --image-uri CLI instead of TEST_IMAGE_URI env.
1 parent 2980ece commit 67c7dac

4 files changed

Lines changed: 9 additions & 22 deletions

File tree

.github/workflows/tensorflow.tests-sagemaker.yml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,15 +75,16 @@ jobs:
7575
- name: Run SageMaker training tests
7676
env:
7777
PYTHONPATH: ${{ github.workspace }}/test
78-
TEST_IMAGE_URI: ${{ needs.preflight.outputs.image-uri }}
7978
SM_ROLE_ARN: arn:aws:iam::${{ needs.preflight.outputs.aws-account-id }}:role/SageMakerRole
8079
run: |
8180
if [ "${{ needs.preflight.outputs.device-type }}" = "gpu" ]; then
82-
pytest test/tensorflow/integration/sagemaker/test_mnist_cuda.py -v
81+
pytest test/tensorflow/integration/sagemaker/test_mnist_cuda.py -v \
82+
--image-uri "${{ needs.preflight.outputs.image-uri }}"
8383
else
8484
pytest \
8585
test/tensorflow/integration/sagemaker/test_mnist_cpu.py \
8686
test/tensorflow/integration/sagemaker/test_experiments_cpu.py \
8787
test/tensorflow/integration/sagemaker/test_tuning_cpu.py \
88-
-v
88+
-v \
89+
--image-uri "${{ needs.preflight.outputs.image-uri }}"
8990
fi

scripts/docker/common/install_efa_amzn2023.sh

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,7 @@ function install_efa {
3636

3737
# EFA 1.48+ auto-detects NGC containers via /opt/nvidia/nvidia_entrypoint.sh
3838
# (present in nvidia/cuda:*-amzn2023 bases) and then skips the AL2023
39-
# libnccl-ofi RPM. Force the standard install path so the OFI-NCCL plugin
40-
# is installed and libnccl-net-ofi.so ends up at /opt/amazon/ofi-nccl/lib64/.
41-
# --disable-ngc was added in EFA 1.48; guard so 1.47 callers still work.
42-
# sort -V (GNU coreutils) provides deterministic semver comparison —
43-
# avoids bash string-order pitfalls like "1.9" > "1.10" being true.
39+
# libnccl-ofi RPM. Force the standard install with --disable-ngc (EFA 1.48+).
4440
ver_ge() {
4541
[ "$1" = "$2" ] || [ "$2" = "$(echo -e "$1\n$2" | sort -V | head -n1)" ]
4642
}

test/tensorflow/integration/sagemaker/conftest.py

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -31,16 +31,6 @@ def source_dir(resource_dir):
3131
# ── Environment / config fixtures ──────────────────────────────────────────
3232

3333

34-
@pytest.fixture(scope="session")
35-
def image_uri():
36-
return os.environ["TEST_IMAGE_URI"]
37-
38-
39-
@pytest.fixture(scope="session")
40-
def default_region():
41-
return "us-west-2"
42-
43-
4434
@pytest.fixture(scope="session")
4535
def mnist_s3_uri():
4636
return "s3://dlc-cicd-models/tensorflow/sagemaker-test-data/MNIST/"
@@ -92,15 +82,15 @@ def _run(
9282

9383

9484
@pytest.fixture(scope="session")
95-
def assert_s3_file_exists(default_region):
85+
def assert_s3_file_exists(region):
9686
"""Return a callable that asserts the given s3:// URL points to an existing object.
9787
9888
head-object via boto3 raises if the key is missing, which surfaces as a
9989
clear test failure when SageMaker didn't upload the model artifact."""
10090

10191
def _assert(s3_url):
10292
parsed_url = urlparse(s3_url)
103-
s3 = boto3.resource("s3", region_name=default_region)
93+
s3 = boto3.resource("s3", region_name=region)
10494
s3.Object(parsed_url.netloc, parsed_url.path.lstrip("/")).load()
10595

10696
return _assert

test/tensorflow/integration/sagemaker/test_experiments_cpu.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,15 @@
3030
INSTANCE_TYPE = "ml.c5.xlarge"
3131

3232

33-
def test_experiments_cpu(image_uri, source_dir, default_region, mnist_s3_uri):
33+
def test_experiments_cpu(image_uri, source_dir, region, mnist_s3_uri):
3434
"""Create an Experiment + Trial, run a training job, associate the
3535
auto-created TrialComponent, then clean everything up.
3636
3737
After `train(wait=True)` returns, we read the training job ARN from
3838
`model_trainer._latest_training_job` (avoiding ListTrainingJobs
3939
eventual-consistency races) and list TrialComponents whose `source_arn`
4040
matches the job ARN to verify auto-creation."""
41-
boto_session = boto3.session.Session(region_name=default_region)
41+
boto_session = boto3.session.Session(region_name=region)
4242
sagemaker_session = Session(boto_session)
4343

4444
experiment_name = random_suffix_name("tf-dlc-integ-test", 63)

0 commit comments

Comments
 (0)