Skip to content

TF-2.16 test modification and handling #4830

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

Merged
merged 28 commits into from
Aug 27, 2024

Conversation

shantanutrip
Copy link
Collaborator

@shantanutrip shantanutrip commented Aug 13, 2024

Issue #, if available:

Description of changes:
TF-2.16 test modification and handling: For TF2.16 DLCs, we have TF2.16.2 training and TF2.16.1 inference images. The counterparts for both of them do not exist which is by-default assumed by the test suite. This causes failures.

TF images do not have a 1:1 connection between training and TF-Serving versions anymore, thus, we have to handle this for TF2.16 images.

To handle this, in Tensorflow Estimator, we just use the major.minor version for TF 2.16.* images when retrieving the image_uri.

This is being handled on the test side by making the tf_full_version point to TF2.16 to ensure tests that use fit and deploy using the same estimator do not fail.

Testing done:
CICD

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • I have read the CONTRIBUTING doc
  • I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
  • I used the commit message format described in CONTRIBUTING
  • I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
  • I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
  • I have checked that my tests are not configured for a specific region or account (if appropriate)
  • I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@shantanutrip shantanutrip requested a review from a team as a code owner August 13, 2024 23:07
@shantanutrip shantanutrip requested a review from ptkab August 13, 2024 23:07
@shantanutrip shantanutrip added the do-not-merge Use when PR needs to be marked as do not merge label Aug 13, 2024
@shantanutrip shantanutrip marked this pull request as draft August 13, 2024 23:08
@@ -555,6 +556,11 @@ def tf_full_version(tensorflow_training_latest_version, tensorflow_inference_lat
Fixture exists as such, since TF training and TFS have different latest versions.
Otherwise, this would simply be a single latest version.
"""
if Version(tensorflow_training_latest_version) in SpecifierSet(">=2.16"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can prob just assign a variable for the version object so you don't have to reinstantiate multiple times

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, made the change

Copy link
Contributor

@arjkesh arjkesh left a comment

Choose a reason for hiding this comment

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

LGTM

@shantanutrip shantanutrip removed the do-not-merge Use when PR needs to be marked as do not merge label Aug 26, 2024
@liujiaorr liujiaorr merged commit d81a2cd into aws:master Aug 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants