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

Added langchain-aws package. #382

Merged
merged 2 commits into from
May 31, 2024
Merged

Conversation

3coins
Copy link
Contributor

@3coins 3coins commented Apr 25, 2024

Description

Adds langchain-aws package.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@andrii-i
Copy link
Contributor

andrii-i commented May 10, 2024

I built sagemaker-distribution based on this PR with aws-langchain included and ran a newly-added test cases with python -m pytest -n auto -vv -rs -k "langchain-aws" --local-image-version 1.8.0, they pass:

Screenshot 2024-05-10 at 3 02 35 PM

Note irregularities with python src/main.py create-minor-version-artifacts --base-patch-version=$BASE_PATCH_VERSION --force command that is supposed to be run after adding {cpu/gpu}.additional_packages_env.in into new version folder accordingly to the docs. It checks if new version folder exists and if it does, checks for existence of {cpu/gpu}.additional_packages_env.in file and pulls new packages into {cpu/gpu}.env.in. But for me it sometimes works incorrectly and overwrites new version folder without checking for {cpu/gpu}.additional_packages_env.in or pulling packages from it.

@3coins 3coins marked this pull request as ready for review May 10, 2024 01:50
@andrii-i
Copy link
Contributor

andrii-i commented May 10, 2024

Note that if all tests are ran, langchain-aws tests pass but tests for 5 other packages fail. For the sake of experiment I tried building 1.8.0 without adding langchain-aws or anything else and I’m getting the same errors.

FAILED test/test_dockerfile_based_harness.py::test_dockerfiles_for_gpu[keras.test.Dockerfile-required_packages0] - assert 1 == 0
FAILED test/test_dockerfile_based_harness.py::test_dockerfiles_for_gpu[scipy.test.Dockerfile-required_packages4] - assert 1 == 0
FAILED test/test_dockerfile_based_harness.py::test_dockerfiles_for_gpu[pandas.test.Dockerfile-required_packages7] - assert 1 == 0
FAILED test/test_dockerfile_based_harness.py::test_dockerfiles_for_gpu[sm-python-sdk.test.Dockerfile-required_packages8] - assert 1 == 0
FAILED test/test_dockerfile_based_harness.py::test_dockerfiles_for_gpu[pytorch.examples.Dockerfile-required_packages9] - assert 1 == 0
===================================================================================== 5 failed, 20 passed, 2 skipped, 4 warnings in 7016.70s (1:56:56) =====================================================================================
Traceback (most recent call last):
File “/local/home/aieroshe/Docs/sagemaker-distribution/src/main.py”, line 432, in
parse_args(get_arg_parser())
File “/local/home/aieroshe/Docs/sagemaker-distribution/src/main.py”, line 428, in parse_args
args.func(args)
File “/local/home/aieroshe/Docs/sagemaker-distribution/src/main.py”, line 193, in build_images
_test_local_images(image_ids, args.target_patch_version)
File “/local/home/aieroshe/Docs/sagemaker-distribution/src/main.py”, line 219, in _test_local_images
assert exit_code == 0, f”Tests failed with exit codes: {exit_codes} against: {image_ids}”
^^^^^^^^^^^^^^
AssertionError: Tests failed with exit codes: [] against: []

All packages for tests of which errors are thrown except sm-python-sdk are listed in v1.8.0/gpu.env.out. At the same time sm-python-sdk is also not listed in v1.7.1/gpu.env.out .

Screenshot 2024-05-10 at 3 01 23 PM


ARG MAMBA_DOCKERFILE_ACTIVATE=1

CMD ["python", "-c", "import langchain_aws"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add more tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for giving this PR a look @balajisankar15. Is this blocking / could we add more tests in the follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balajisankar15
We can add more tests around importing other sub-modules to test their importability. Anything other than that requires an elaborate setup with boto3 credentials and service endpoint infrastructure (bedrock, kendra, vectorstores etc.) to test each of these components, which we don't plan to do for this library.
We will update the PR with these and open for a review. Thanks.

@3coins
Copy link
Contributor Author

3coins commented May 20, 2024

@balajisankar15
Added more import tests to the test file. Can you take a look. Thanks.

@andrii-i andrii-i force-pushed the add-langchain-aws branch from b6ed925 to 89e875f Compare May 22, 2024 23:50
@andrii-i
Copy link
Contributor

@balajisankar15 @claytonparnell
Rebased this PR on main and updated the path of {cpu/gpu}.additional_packages_env.in to build_artifacts/v1/v1.9/v1.9.0/. I also built an image and ran langchain-aws tests, both successfully. Please give this another look.

@3coins
Copy link
Contributor Author

3coins commented May 31, 2024

@claytonparnell @balajisankar15
Is there a plan to merge this PR, is there an ETA that you can provide?

@claytonparnell claytonparnell merged commit 6825fbe into aws:main May 31, 2024
1 check 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