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

[SageMaker FeatureStore] BatchGetRecord does not work #1461

Closed
3 tasks done
momilo opened this issue Oct 17, 2021 · 6 comments
Closed
3 tasks done

[SageMaker FeatureStore] BatchGetRecord does not work #1461

momilo opened this issue Oct 17, 2021 · 6 comments
Labels
bug This issue is a bug. closed-for-staleness service-api This issue is due to a problem in a service API, not the SDK implementation.

Comments

@momilo
Copy link

momilo commented Oct 17, 2021

Describe the bug

There is a major bug in Sagemaker FeatureStore Client. When using BatchGetRecord, it always returns an empty slice of records and nil error, making the function unusable at all.

Bug Analysis

Debugging http response reveals that AWS API returns the following response:
{"Output":{"__type":"com.amazon.coral.service#UnknownOperationException"},"Version":"1.0"}

Detailed analysis reveals that:

  • Go SDK (aws-sdk-go-v2) specifies in the request headers content-type as application/json; this is in line with the (unfortunately erroneous) documentation
  • Python SDK and AWS CLI, which do work on the very same machine, send the request with no content-type specified, and successfully return the values expected

Recommendation

Immediate Fix Recommendation

It can be shown in a replicable manner that by removing/commenting-out line 47 in service/sagemakerfeaturestoreruntime/serializers.go , the request succeeds.

It is therefore recommended that the API specification in aws-sdk-go-v2 is aligned with the actual API.

I am not entirely familiar with Smithy, but I would imagine that this could be easily achieved by adding an appropriate override to awsmodels/sagemakerfeaturestoreruntime2020-07-01.json, specifically to traits of the method, and re-generating the code, to ensure that the aforementioned line in serializers.go is removed.

Long Term Fix Recommendation

After a quick-fix is implemented, it is recommended to start the (most likely longer) process of informing the Sagemaker team about the bug in their api request validation implementation, and to align aws cli and python sdk with the documentation.

Misc

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Version of AWS SDK for Go?

The most current master, Release (2021-10-11), and also a number of previous releases.

Version of Go (go version)?

1.16, 1.17

To Reproduce (observed behaviour)

Simply invoke BatchGetRecord method on an existing FeatureStore, with the appropriate region specified upon client initialisation.

out, err := client.BatchGetRecord(ctx, &sagemakerfeaturestoreruntime.BatchGetRecordInput{
		Identifiers: []types.BatchGetRecordIdentifier{
			{
				FeatureGroupName:               aws.String("myfancystore"),
				RecordIdentifiersValueAsString: recordIDs,
				FeatureNames:                   nil,
			},
		},
	})

Expected behaviour
I would expect features to be retrieved or an error to be returned. Instead, I receive an empty slice of records - despite the Feature Store being correctly populated. Debugging reveals as per above.

@momilo momilo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 17, 2021
@jasdel
Copy link
Contributor

jasdel commented Oct 19, 2021

Thanks for taking the time and reporting this issue @momilo. We're investigating this issue, and how it can be fixed. Its possible this is similar to a set of fixes we made to the v1 SDK last week, aws/aws-sdk-go#4116

@nateprewitt
Copy link
Member

Hi @momilo,

I wanted to follow up with a quick question while we're looking into this. For Go v2, is this issue replicated across all SageMaker FeatureStore APIs, or specific to this Batch operation?

@KaibaLopez KaibaLopez added service-api This issue is due to a problem in a service API, not the SDK implementation. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 19, 2021
@momilo
Copy link
Author

momilo commented Oct 19, 2021

It works for other APIs (I've tried PutRecord, GetRecord). It seems to me that the API simply rejects a request with application/json for this specific call.

This is not unthinkable (from a "bug in request validation" perspective) - keep in mind that:

  • PutRecord, GetRecord, DeleteRecord - are invoked on a feature-group-specific subpath /FeatureGroup/FeatureGroupName
  • BatchGetRecord is invoked on the main (host) path (/) of the FeatureStore API

@nateprewitt
Copy link
Member

Thanks for the confirmation @momilo! I was able to identify the bug in SageMaker FeatureStore and we have the service team working on it currently. The current behavior of Go v2 (and now Go v1) is correct in this case.

Python is actually slated to move to sending application/json in service requests as its current behavior is considered incorrect. You can track progress on that here. We'll do some additional sanity testing to identify any remaining outliers.

For Go v2's behavior, I'll defer the decision to the Go team pending the timeline we receive from SageMaker.

@momilo
Copy link
Author

momilo commented Oct 19, 2021

Thank you very much for your kind help. I totally agree that it's technically a service bug, not SDK bug. Let's hope they can fix it quickly. If not - I would appreciate you re-considering to do a temporary hotfix in the SDK.

@nateprewitt
Copy link
Member

Hi @momilo, this should be fixed with SageMaker FeatureStore now. We've merged and released the changes in the CLI 1.21.0/Boto3 1.19.0 as well. Please let us know if you're still experiencing issues, otherwise, this should be set to resolve.

@kellertk kellertk added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 17, 2021
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. closed-for-staleness service-api This issue is due to a problem in a service API, not the SDK implementation.
Projects
None yet
Development

No branches or pull requests

5 participants