Open
Conversation
nargokul
reviewed
Mar 10, 2026
sagemaker-mlops/src/sagemaker/mlops/feature_store/feature_group.py
Outdated
Show resolved
Hide resolved
sagemaker-mlops/src/sagemaker/mlops/feature_store/feature_group_manager.py
Show resolved
Hide resolved
6f00f8a to
3baff6c
Compare
3baff6c to
e706a5c
Compare
Remove _lf_client_cache and _s3_client_cache instance caches from _get_lake_formation_client and _get_s3_client. Each call now creates a fresh boto3 client directly. Remove corresponding cache-specific unit tests (cache reuse and different-region tests). --- X-AI-Prompt: remove client caching for lf and s3 in feature_group_manager and update tests X-AI-Tool: kiro-cli
Add acknowledge_risk: Optional[bool] = None to enable_lake_formation() and LakeFormationConfig. None triggers interactive input() prompt, True proceeds without prompting, False aborts with RuntimeError. Removes all builtins.input mocking from unit and integration tests. Tests now pass acknowledge_risk=True or False directly. Removes one duplicate test that became identical after the refactor. --- X-AI-Prompt: add y/n confirmation for disable_hybrid_access_mode=True, then refactor to use acknowledge_risk param instead of input() X-AI-Tool: kiro-cli
952fab7 to
d926cbb
Compare
4ae73ab to
7f6e22b
Compare
7f6e22b to
d9f3e55
Compare
- Use assumed role session for lf_client, glue_client, and athena_client instead of default boto3 session - Move client initialization to setup/configuration cell - Add session=boto_session to get_record in Example 2 - Fix print statements: "execution role" -> "offline store role" - Remove unused get_execution_role import - Remove misleading LakeFormationDataLakeAdmin comment - Fix typo: "Exectution" -> "Execution" - Fix PascalCase variables to snake_case - Fix "lakeformation" -> "Lake Formation" in markdown - Fix bold markdown formatting - Add missing space in ARN print - Remove duplicate boto3 and time imports - Scope cleanup IAM policy to lf-demo-* resources - Fix cleanup variable to use correct reference - Remove empty trailing markdown cell
d9f3e55 to
d12f036
Compare
nargokul
approved these changes
Apr 10, 2026
mollyheamazon
approved these changes
Apr 10, 2026
| "Hybrid access mode is not disabled. IAM-based access to the Glue table will " | ||
| "still be allowed. Do you want to proceed without revoking IAMAllowedPrincipal " | ||
| "permissions? (y/n): " | ||
| ).strip().lower() == "y" |
Contributor
There was a problem hiding this comment.
input() is not safe for non-interactive environments (CI, pipelines, SageMaker jobs).
Please require acknowledge_risk to be explicitly set to True or False and remove the None interactive path.
| proceed = input( | ||
| "This will revoke IAMAllowedPrincipal permissions and may break existing jobs " | ||
| "that rely on IAM-based access. Do you want to proceed? (y/n): " | ||
| ).strip().lower() == "y" |
Contributor
There was a problem hiding this comment.
Same comment as above:
input() is not safe for non-interactive environments (CI, pipelines, SageMaker jobs).
Please require acknowledge_risk to be explicitly set to True or False and remove the None interactive path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds Lake Formation integration to SageMaker Feature Store, enabling customers to govern access to their offline store data through AWS Lake Formation instead of relying solely on IAM policies.
This simplifies the manual process described in this blog
https://aws.amazon.com/blogs/machine-learning/control-access-to-amazon-sagemaker-feature-store-offline-using-aws-lake-formation/
New Features
LakeFormationConfig— declarative configuration for Lake Formation governance:FeatureGroupManager.create()— added lake_formation_config parameterFeatureGroupManager.enable_lake_formation()— new methodEnables Lake Formation on existing Feature Groups
Three-phase setup:
Fail-fast behavior with clear error reporting at each phase
Interactive confirmation prompts warn users about risks (controllable via acknowledge_risk)
Logs recommended S3 deny policy as a warning
Usage
Enable at creation:
Testing
Notes
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.