fix: Redundant feature materialization and premature incremental materialization timestamp updates by james-crabtree-sp · Pull Request #3789 · feast-dev/feast

james-crabtree-sp · 2023-10-09T20:42:40Z

What this PR does / why we need it:

This PR fixes 2 issues encountered while attempting to run very large materializations (Several billion rows as part of an initial run of online materialization).

Every pod created materializes every file in the dataset. This is redundant and costs a lot of time and money. To fix this, I updated the dataflow code to select only the single file corresponding to the running pod's JOB_COMPLETION_INDEX as input.
The last materialized date used for incremental materialization was being updated before materialization had even run. This meant that if materialization had failed for some reason, many updates could be ignored, because the next run would continue from beyond the end date of the failed run. I solved this by implementing a synchronous mode for bytewax materialization that will wait for materialization of a feature to complete successfully before returning and updating the last materialized date
Bytewax jobs can run infinitely if node crashes occur often enough that the job has to continuously reprocess the same pods after they are lost. I mitigated this by adding a configurable activeDeadlineSeconds and by adding an option to synchronous materialization to allow these jobs to be split into smaller batches so fewer pods have to be rerun in the event of a node failure

Which issue(s) this PR fixes:
Fixes #3786
Fixes #3787
Fixes #3788

sudohainguyen · 2023-10-11T06:26:14Z

great, I have been working on the similar PR, but you did it first and way better 💯

sdk/python/feast/infra/materialization/contrib/bytewax/bytewax_materialization_engine.py

whoahbot · 2023-10-12T14:59:43Z

@james-crabtree-sp Thanks again for putting this PR together. I think in order to merge, you'll need to sign your commits: https://docs.feast.dev/project/development-guide#signing-off-commits

sdk/python/feast/infra/materialization/contrib/bytewax/bytewax_materialization_engine.py

james-crabtree-sp · 2023-10-12T15:41:40Z

@james-crabtree-sp Thanks again for putting this PR together. I think in order to merge, you'll need to sign your commits: https://docs.feast.dev/project/development-guide#signing-off-commits

I think they are all signed off already. I see that the DCO task that checks this completed successfully https://github.com/feast-dev/feast/pull/3789/checks?check_run_id=17541354092

whoahbot · 2023-10-19T15:49:47Z

@james-crabtree-sp Thanks again for putting this PR together. I think in order to merge, you'll need to sign your commits: https://docs.feast.dev/project/development-guide#signing-off-commits

I think they are all signed off already. I see that the DCO task that checks this completed successfully https://github.com/feast-dev/feast/pull/3789/checks?check_run_id=17541354092

Sorry for the confusion, I was referring to the requirements to merge, and linked to the wrong documentation.

This is the requirement that I was referring to:

Merging is blocked
The base branch requires all commits to be signed. Learn more about signing commits.

sudohainguyen · 2023-10-21T05:17:10Z

hey @james-crabtree-sp this is a great PR, you need any hands to make it merged? I see there's a lot of commits in the past unverified

if you don't mind I can create another PR to mimic this one

Signed-off-by: James Crabtree <[email protected]>

) * SAASMLOPS-809 fix bytewax workers so they only process a single file * SAASMLOPS-809 fix newlines Signed-off-by: James Crabtree <[email protected]>

* SAASMLOPS-833 add configurable job timeout * SAASMLOPS-833 fix whitespace Signed-off-by: James Crabtree <[email protected]>

Signed-off-by: James Crabtree <[email protected]>

james-crabtree-sp · 2023-10-23T16:45:26Z

@whoahbot @sudohainguyen Ahh, I understand now. Commits are now verified

sudohainguyen · 2023-10-23T16:49:57Z

@whoahbot @sudohainguyen Ahh, I understand now. Commits are now verified

exactly 🤣 looks good now but you need to run code format though

james-crabtree-sp · 2023-10-23T16:52:14Z

@whoahbot @sudohainguyen Ahh, I understand now. Commits are now verified

exactly 🤣 looks good now but you need to run code format though

Noticed that too. Weird rebase issue. Should be fixed now

…error Signed-off-by: James Crabtree <[email protected]>

Signed-off-by: James Crabtree <[email protected]>

sudohainguyen · 2023-10-25T01:51:53Z

@james-crabtree-sp I've been adopting your changes on my side

so far do we need to wait the job to be done, like forever without timeout?

james-crabtree-sp · 2023-10-25T03:05:58Z

@james-crabtree-sp I've been adopting your changes on my side

so far do we need to wait the job to be done, like forever without timeout?

Well, this PR adds the configurable active_deadline_seconds setting, which can be used to impose a timeout on the kubernetes jobs. Does that answer your question?

sudohainguyen · 2023-10-25T03:12:21Z

@james-crabtree-sp I've been adopting your changes on my side
so far do we need to wait the job to be done, like forever without timeout?

Well, this PR adds the configurable active_deadline_seconds setting, which can be used to impose a timeout on the kubernetes jobs. Does that answer your question?

cool clear

# [0.35.0](v0.34.0...v0.35.0) (2024-01-13) ### Bug Fixes * Add async refresh to prevent synchronous refresh in main thread ([#3812](#3812)) ([9583ed6](9583ed6)) * Adopt connection pooling for HBase ([#3793](#3793)) ([b3852bf](b3852bf)) * Bytewax engine create configmap from object ([#3821](#3821)) ([25e9775](25e9775)) * Fix warnings from deprecated paths and update default log level ([#3757](#3757)) ([68a8737](68a8737)) * improve parsing bytewax job status ([5983f40](5983f40)) * make bytewax settings unexposed ([ae1bb8b](ae1bb8b)) * Make generated temp table name escaped ([#3797](#3797)) ([175d796](175d796)) * Pin numpy version to avoid spammy deprecation messages ([774ed33](774ed33)) * Redundant feature materialization and premature incremental materialization timestamp updates ([#3789](#3789)) ([417b16b](417b16b)), closes [#6](#6) [#7](#7) * Resolve hbase hotspot issue when materializing ([#3790](#3790)) ([7376db8](7376db8)) * Set keepalives_idle None by default ([#3756](#3756)) ([8717e9b](8717e9b)) * Set upper bound for bigquery client due to its breaking changes ([2151c39](2151c39)) * UI project cannot handle fallback routes ([#3766](#3766)) ([96ece0f](96ece0f)) * update dependencies versions due to conflicts ([5dc0b24](5dc0b24)) * Update jackson and remove unnecessary logging ([#3809](#3809)) ([018d0ea](018d0ea)) * upgrade the pyarrow to latest v14.0.1 for CVE-2023-47248. ([052182b](052182b)) ### Features * Add get online feature rpc to gprc server ([#3815](#3815)) ([01db8cc](01db8cc)) * Add materialize and materialize-incremental rest endpoints ([#3761](#3761)) ([fa600fe](fa600fe)), closes [#3760](#3760) * add redis sentinel support ([3387a15](3387a15)) * add redis sentinel support ([4337c89](4337c89)) * add redis sentinel support format lint ([aad8718](aad8718)) * Add support for `table_create_disposition` in bigquery job for offline store ([#3762](#3762)) ([6a728fe](6a728fe)) * Add support for in_cluster config and additional labels for bytewax materialization ([#3754](#3754)) ([2192e65](2192e65)) * Apply cache to load proto registry for performance ([#3702](#3702)) ([709c709](709c709)) * Make bytewax job write as mini-batches ([#3777](#3777)) ([9b0e5ce](9b0e5ce)) * Optimize bytewax pod resource with zero-copy ([9cf9d96](9cf9d96)) * Support GCS filesystem for bytewax engine ([#3774](#3774)) ([fb6b807](fb6b807))

# [0.35.0](feast-dev/feast@v0.34.0...v0.35.0) (2024-01-13) ### Bug Fixes * Add async refresh to prevent synchronous refresh in main thread ([feast-dev#3812](feast-dev#3812)) ([9583ed6](feast-dev@9583ed6)) * Adopt connection pooling for HBase ([feast-dev#3793](feast-dev#3793)) ([b3852bf](feast-dev@b3852bf)) * Bytewax engine create configmap from object ([feast-dev#3821](feast-dev#3821)) ([25e9775](feast-dev@25e9775)) * Fix warnings from deprecated paths and update default log level ([feast-dev#3757](feast-dev#3757)) ([68a8737](feast-dev@68a8737)) * improve parsing bytewax job status ([5983f40](feast-dev@5983f40)) * make bytewax settings unexposed ([ae1bb8b](feast-dev@ae1bb8b)) * Make generated temp table name escaped ([feast-dev#3797](feast-dev#3797)) ([175d796](feast-dev@175d796)) * Pin numpy version to avoid spammy deprecation messages ([774ed33](feast-dev@774ed33)) * Redundant feature materialization and premature incremental materialization timestamp updates ([feast-dev#3789](feast-dev#3789)) ([417b16b](feast-dev@417b16b)), closes [feast-dev#6](feast-dev#6) [feast-dev#7](feast-dev#7) * Resolve hbase hotspot issue when materializing ([feast-dev#3790](feast-dev#3790)) ([7376db8](feast-dev@7376db8)) * Set keepalives_idle None by default ([feast-dev#3756](feast-dev#3756)) ([8717e9b](feast-dev@8717e9b)) * Set upper bound for bigquery client due to its breaking changes ([2151c39](feast-dev@2151c39)) * UI project cannot handle fallback routes ([feast-dev#3766](feast-dev#3766)) ([96ece0f](feast-dev@96ece0f)) * update dependencies versions due to conflicts ([5dc0b24](feast-dev@5dc0b24)) * Update jackson and remove unnecessary logging ([feast-dev#3809](feast-dev#3809)) ([018d0ea](feast-dev@018d0ea)) * upgrade the pyarrow to latest v14.0.1 for CVE-2023-47248. ([052182b](feast-dev@052182b)) ### Features * Add get online feature rpc to gprc server ([feast-dev#3815](feast-dev#3815)) ([01db8cc](feast-dev@01db8cc)) * Add materialize and materialize-incremental rest endpoints ([feast-dev#3761](feast-dev#3761)) ([fa600fe](feast-dev@fa600fe)), closes [feast-dev#3760](feast-dev#3760) * add redis sentinel support ([3387a15](feast-dev@3387a15)) * add redis sentinel support ([4337c89](feast-dev@4337c89)) * add redis sentinel support format lint ([aad8718](feast-dev@aad8718)) * Add support for `table_create_disposition` in bigquery job for offline store ([feast-dev#3762](feast-dev#3762)) ([6a728fe](feast-dev@6a728fe)) * Add support for in_cluster config and additional labels for bytewax materialization ([feast-dev#3754](feast-dev#3754)) ([2192e65](feast-dev@2192e65)) * Apply cache to load proto registry for performance ([feast-dev#3702](feast-dev#3702)) ([709c709](feast-dev@709c709)) * Make bytewax job write as mini-batches ([feast-dev#3777](feast-dev#3777)) ([9b0e5ce](feast-dev@9b0e5ce)) * Optimize bytewax pod resource with zero-copy ([9cf9d96](feast-dev@9cf9d96)) * Support GCS filesystem for bytewax engine ([feast-dev#3774](feast-dev#3774)) ([fb6b807](feast-dev@fb6b807)) Signed-off-by: tokoko <[email protected]>

…rialization timestamp updates (feast-dev#3789) * SAASMLOPS-767 wait for jobs to complete Signed-off-by: James Crabtree <[email protected]> * SAASMLOPS-805 Stopgap change to fix duplicate materialization of data Signed-off-by: James Crabtree <[email protected]> * SAASMLOPS-805 save BYTEWAX_REPLICAS=1 Signed-off-by: James Crabtree <[email protected]> * SAASMLOPS-809 fix bytewax workers so they only process a single file (feast-dev#6) * SAASMLOPS-809 fix bytewax workers so they only process a single file * SAASMLOPS-809 fix newlines Signed-off-by: James Crabtree <[email protected]> * SAASMLOPS-833 add configurable job timeout (feast-dev#7) * SAASMLOPS-833 add configurable job timeout * SAASMLOPS-833 fix whitespace Signed-off-by: James Crabtree <[email protected]> * develop Run large materializations in batches of pods Signed-off-by: James Crabtree <[email protected]> * master Set job_batch_size at least equal to max_parallelism Signed-off-by: James Crabtree <[email protected]> * master clarity max_parallelism description Signed-off-by: James Crabtree <[email protected]> * master resolve bug that causes materialization to continue after job error Signed-off-by: James Crabtree <[email protected]> * master resolve bug causing pod logs to not be printed Signed-off-by: James Crabtree <[email protected]> --------- Signed-off-by: James Crabtree <[email protected]> Signed-off-by: Attila Toth <[email protected]>

# [0.35.0](feast-dev/feast@v0.34.0...v0.35.0) (2024-01-13) ### Bug Fixes * Add async refresh to prevent synchronous refresh in main thread ([feast-dev#3812](feast-dev#3812)) ([9583ed6](feast-dev@9583ed6)) * Adopt connection pooling for HBase ([feast-dev#3793](feast-dev#3793)) ([b3852bf](feast-dev@b3852bf)) * Bytewax engine create configmap from object ([feast-dev#3821](feast-dev#3821)) ([25e9775](feast-dev@25e9775)) * Fix warnings from deprecated paths and update default log level ([feast-dev#3757](feast-dev#3757)) ([68a8737](feast-dev@68a8737)) * improve parsing bytewax job status ([5983f40](feast-dev@5983f40)) * make bytewax settings unexposed ([ae1bb8b](feast-dev@ae1bb8b)) * Make generated temp table name escaped ([feast-dev#3797](feast-dev#3797)) ([175d796](feast-dev@175d796)) * Pin numpy version to avoid spammy deprecation messages ([774ed33](feast-dev@774ed33)) * Redundant feature materialization and premature incremental materialization timestamp updates ([feast-dev#3789](feast-dev#3789)) ([417b16b](feast-dev@417b16b)), closes [feast-dev#6](feast-dev#6) [feast-dev#7](feast-dev#7) * Resolve hbase hotspot issue when materializing ([feast-dev#3790](feast-dev#3790)) ([7376db8](feast-dev@7376db8)) * Set keepalives_idle None by default ([feast-dev#3756](feast-dev#3756)) ([8717e9b](feast-dev@8717e9b)) * Set upper bound for bigquery client due to its breaking changes ([2151c39](feast-dev@2151c39)) * UI project cannot handle fallback routes ([feast-dev#3766](feast-dev#3766)) ([96ece0f](feast-dev@96ece0f)) * update dependencies versions due to conflicts ([5dc0b24](feast-dev@5dc0b24)) * Update jackson and remove unnecessary logging ([feast-dev#3809](feast-dev#3809)) ([018d0ea](feast-dev@018d0ea)) * upgrade the pyarrow to latest v14.0.1 for CVE-2023-47248. ([052182b](feast-dev@052182b)) ### Features * Add get online feature rpc to gprc server ([feast-dev#3815](feast-dev#3815)) ([01db8cc](feast-dev@01db8cc)) * Add materialize and materialize-incremental rest endpoints ([feast-dev#3761](feast-dev#3761)) ([fa600fe](feast-dev@fa600fe)), closes [feast-dev#3760](feast-dev#3760) * add redis sentinel support ([3387a15](feast-dev@3387a15)) * add redis sentinel support ([4337c89](feast-dev@4337c89)) * add redis sentinel support format lint ([aad8718](feast-dev@aad8718)) * Add support for `table_create_disposition` in bigquery job for offline store ([feast-dev#3762](feast-dev#3762)) ([6a728fe](feast-dev@6a728fe)) * Add support for in_cluster config and additional labels for bytewax materialization ([feast-dev#3754](feast-dev#3754)) ([2192e65](feast-dev@2192e65)) * Apply cache to load proto registry for performance ([feast-dev#3702](feast-dev#3702)) ([709c709](feast-dev@709c709)) * Make bytewax job write as mini-batches ([feast-dev#3777](feast-dev#3777)) ([9b0e5ce](feast-dev@9b0e5ce)) * Optimize bytewax pod resource with zero-copy ([9cf9d96](feast-dev@9cf9d96)) * Support GCS filesystem for bytewax engine ([feast-dev#3774](feast-dev#3774)) ([fb6b807](feast-dev@fb6b807)) Signed-off-by: Attila Toth <[email protected]>

james-crabtree-sp requested a review from whoahbot as a code owner October 9, 2023 20:42

james-crabtree-sp changed the title ~~fix: redundant feature materialization and premature incremental materialization timestamp updates~~ fix: Redundant feature materialization and premature incremental materialization timestamp updates Oct 9, 2023

whoahbot approved these changes Oct 10, 2023

View reviewed changes

whoahbot added lgtm ok-to-test labels Oct 10, 2023

sudohainguyen reviewed Oct 11, 2023

View reviewed changes

sdk/python/feast/infra/materialization/contrib/bytewax/bytewax_materialization_engine.py Show resolved Hide resolved

sudohainguyen reviewed Oct 12, 2023

View reviewed changes

sdk/python/feast/infra/materialization/contrib/bytewax/bytewax_materialization_engine.py Show resolved Hide resolved

KarolisKont mentioned this pull request Oct 17, 2023

fix: Redundant feature materialization and premature incremental materialization timestamp updates vinted/feast#4

Merged

whoahbot added the approved label Oct 19, 2023

james-crabtree-sp force-pushed the master branch 2 times, most recently from 0631425 to baf108c Compare October 23, 2023 16:37

james-crabtree-sp and others added 8 commits October 23, 2023 11:39

SAASMLOPS-767 wait for jobs to complete

948c7fb

Signed-off-by: James Crabtree <[email protected]>

SAASMLOPS-805 Stopgap change to fix duplicate materialization of data

0ec030f

Signed-off-by: James Crabtree <[email protected]>

SAASMLOPS-805 save BYTEWAX_REPLICAS=1

71c02f0

Signed-off-by: James Crabtree <[email protected]>

SAASMLOPS-809 fix bytewax workers so they only process a single file (#6

2313c1d

) * SAASMLOPS-809 fix bytewax workers so they only process a single file * SAASMLOPS-809 fix newlines Signed-off-by: James Crabtree <[email protected]>

SAASMLOPS-833 add configurable job timeout (#7)

5f97ed0

* SAASMLOPS-833 add configurable job timeout * SAASMLOPS-833 fix whitespace Signed-off-by: James Crabtree <[email protected]>

develop Run large materializations in batches of pods

65c039a

Signed-off-by: James Crabtree <[email protected]>

master Set job_batch_size at least equal to max_parallelism

cbda2bf

Signed-off-by: James Crabtree <[email protected]>

master clarity max_parallelism description

c35b0c1

Signed-off-by: James Crabtree <[email protected]>

james-crabtree-sp force-pushed the master branch from baf108c to fabd417 Compare October 23, 2023 16:44

james-crabtree-sp force-pushed the master branch from fabd417 to c35b0c1 Compare October 23, 2023 16:50

master resolve bug that causes materialization to continue after job …

5a8018d

…error Signed-off-by: James Crabtree <[email protected]>

master resolve bug causing pod logs to not be printed

938b903

Signed-off-by: James Crabtree <[email protected]>

james-crabtree-sp force-pushed the master branch from 9920df7 to 938b903 Compare October 25, 2023 01:38

achals merged commit 417b16b into feast-dev:master Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Redundant feature materialization and premature incremental materialization timestamp updates#3789

fix: Redundant feature materialization and premature incremental materialization timestamp updates#3789
achals merged 10 commits intofeast-dev:masterfrom
sailpoint:master

james-crabtree-sp commented Oct 9, 2023

Uh oh!

sudohainguyen commented Oct 11, 2023

Uh oh!

Uh oh!

whoahbot commented Oct 12, 2023

Uh oh!

Uh oh!

james-crabtree-sp commented Oct 12, 2023

Uh oh!

whoahbot commented Oct 19, 2023

Uh oh!

sudohainguyen commented Oct 21, 2023

Uh oh!

james-crabtree-sp commented Oct 23, 2023

Uh oh!

sudohainguyen commented Oct 23, 2023

Uh oh!

james-crabtree-sp commented Oct 23, 2023

Uh oh!

sudohainguyen commented Oct 25, 2023

Uh oh!

james-crabtree-sp commented Oct 25, 2023

Uh oh!

sudohainguyen commented Oct 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

james-crabtree-sp commented Oct 9, 2023

Uh oh!

sudohainguyen commented Oct 11, 2023

Uh oh!

Uh oh!

whoahbot commented Oct 12, 2023

Uh oh!

Uh oh!

james-crabtree-sp commented Oct 12, 2023

Uh oh!

whoahbot commented Oct 19, 2023

Uh oh!

sudohainguyen commented Oct 21, 2023

Uh oh!

james-crabtree-sp commented Oct 23, 2023

Uh oh!

sudohainguyen commented Oct 23, 2023

Uh oh!

james-crabtree-sp commented Oct 23, 2023

Uh oh!

sudohainguyen commented Oct 25, 2023

Uh oh!

james-crabtree-sp commented Oct 25, 2023

Uh oh!

sudohainguyen commented Oct 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments