feat: Add support for DynamoDB online_read in batches

[TremaMiguel]

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #2247

Context

Given a list of entity_ids set a batch_size parameter to determine the number of items to send in a batch_get_item request to DynamoDB.

DynamoDB can retrieve up to 16MB of data and the record size limit is 400kb, batch_size value could be at most 40.

[codecov-commenter]

Codecov Report

Merging #2371 (c7ab086) into master (45db6dc) will increase coverage by 0.09%.
The diff coverage is 97.64%.

@@            Coverage Diff             @@
##           master    #2371      +/-   ##
==========================================
+ Coverage   83.54%   83.64%   +0.09%     
==========================================
  Files         123      125       +2     
  Lines       10749    10820      +71     
==========================================
+ Hits         8980     9050      +70     
- Misses       1769     1770       +1     

Flag	Coverage Δ
integrationtests	72.97% <75.60%> (-0.26%)	⬇️
unittests	58.03% <95.29%> (+0.49%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdk/python/tests/utils/online_store_utils.py	90.47% <90.47%> (ø)
sdk/python/feast/infra/online_stores/dynamodb.py	88.12% <100.00%> (+1.79%)	⬆️
...ts/unit/online_store/test_dynamodb_online_store.py	100.00% <100.00%> (ø)
.../integration/online_store/test_universal_online.py	71.05% <0.00%> (+0.23%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45db6dc...c7ab086. Read the comment docs.

[TremaMiguel]

@adchia @vlin-lgtm I didn't found any tests for DynamoDBOnlineStore methods. I'm thinking about adding some using moto.

[woop]

@TremaMiguel thanks for this. Would you mind changing your PR title to some kind of explaination of what the PR does? Something like "Add support for DynamoDB reads and writes in batches". This will become a part of our changelog.

[adchia]

Another thing is to not use git pull and instead use eg git pull --rebase (you also should rebase changes against master again since there were test breakages that now are fixed)

as far as tests go, technically there is a sets of tests that call into dynamo (test_universal_online for example). Though I don't think there's a test that does a batch fetch.

moto looks interesting though. might be a good idea to use that so we can let contributors run tests locally for aws. would love to see how that looks.

[TremaMiguel]

@adchia added a test for online_read method parametrized to test 5, 50 or 100 samples at a time.

To run these test AWS credential shoud be set

export AWS_ACCESS_KEY_ID=your_access_key_id
export AWS_SECRET_ACCESS_KEY=your_secret_access_key

I think we can open an issue to improve tests for DynamoDBOnlineStore, especially online_write_batch.

[TremaMiguel]

@adchia I've changed tests to mock dynamodb behavior through moto, no need to set AWS credentials.

Some tests are failing due to #2376

[vlin-lgtm]

Can we hide batch_size from users? So something like this can work:

batch_size = 40
entity_ids = ...
batches = []
for i in range(0, len(entity_ids), batch_size): 
  batches.append(entity_ids[i:i + batch_size])

for batch in batches:
  batch_entity_ids = ...
  ...
  ...
  ...
  response = dynamodb_resource.batch_get_item(RequestItems=batch_entity_ids)
  ...

So this always tries to send at most 40 entities at a time. It is better to maximize the batch size as much as possible so there is less network calls to DynamoDB.

[TremaMiguel]

@adchia could you provide guidance on how to run integration tests locally for AWS? It appears there's something missing in CONTRIBUTING.md

AWS
TODO(adchia): flesh out setting up AWS login (or create helper script)
Modify RedshiftDataSourceCreator to use your credentials

[adchia]

@adchia could you provide guidance on how to run integration tests locally for AWS? It appears there's something missing in CONTRIBUTING.md

AWS
TODO(adchia): flesh out setting up AWS login (or create helper script)
Modify RedshiftDataSourceCreator to use your credentials

Hey!

So what I'd actually do is make a new repo_configuration that uses a local offline store + dynamo as the online store. That's configured at https://github.com/feast-dev/feast/blob/master/sdk/python/tests/integration/feature_repos/repo_configuration.py#L88-L88

You can basically just ignore all these and set
DEFAULT_FULL_REPO_CONFIGS = [IntegrationTestRepoConfig(online_store=DYNAMO_CONFIG)]. Then as long as you're properly authenticated to be able to create / teardown new dynamo tables, then tests should work fine.

Normally to run everything, you'd run make test-python-universal, but in this case the only test failures are in test_universal_online, so you can just do:

FEAST_USAGE=False IS_TEST=True python -m pytest -n 8 --integration --universal sdk/python/tests/integration/online_store/

[TremaMiguel]

@adchia some integrations tests expect results from DynamoDB query to have the same order as input entity_keys, I was reading that BatchGetItem don't return results in order.

I added a sort_response parameter that defaults to True to sort DynamoDB results in the same order as the input entity_keys

Do you have any suggestions to improve the implementation of _sort_dynamodb_response method?

[TremaMiguel]

@adchia test_online_store_cleanup with the case Redshift as offline store and DynamoDB as online store fails.

After doing some digging, there's a difference in the result of fs.get_online_features operation.

1. The first one is to check if features are available, returning driver_id and value as results values.

metadata {
  feature_names {
    val: "driver_id"
    val: "value"
  }
}
results {
  values {
    int64_val: 5001
  }
  values {
    float_val: 0.03799890726804733
  }
  statuses: PRESENT
  statuses: PRESENT
  event_timestamps {
  }
  event_timestamps {
    seconds: 1647104400
  }
}
results {
  values {
    int64_val: 5002
  }
  values {
    float_val: 0.8108303546905518
  }
  statuses: PRESENT
  statuses: PRESENT
  event_timestamps {
  }
  event_timestamps {
    seconds: 1647104400
  }
}

2. Second case check if features for other are available after one FeatureView is deleted, results are similar as the previous one.

3. This one is different, the third case verify that features for other features are deleted, the result from fs.get_online_features is missing the values from the "value" feature

metadata {
  feature_names {
    val: "driver_id"
    val: "value"
  }
}
results {
  values {
    int64_val: 5001
  }
  values {
  }
  statuses: PRESENT
  statuses: NOT_FOUND
  event_timestamps {
  }
  event_timestamps {
  }
}
results {
  values {
    int64_val: 5002
  }
  statuses: PRESENT
  event_timestamps {
  }
}
results {
  values {
    int64_val: 5003
  }
  statuses: PRESENT
  event_timestamps {
  }
}

do you have an idea why is this happening?

[TremaMiguel]

@adchia just to follow up the conversations, is there something you'd like to add?

[adchia]

/lgtm

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adchia, TremaMiguel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [adchia]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vlin-lgtm]

I'm late in the game, but I'd like to thank you @TremaMiguel very much for contributing this! This is going improve our feature serving latency by a great deal! ❤️

[TremaMiguel]

Thanks @vlin-lgtm, I believe there's room for improvement related to unit testing the DynamoDb Online store module, we could use something like moto for this, as we've done with the online_read method.

Having these unitary tests plus the local integration tests would definitively help make the code more robust and facilitate future changes.