Better logging for materialize command #1499
Conversation
sdk/python/feast/infra/gcp.py
Outdated
| lambda b: _write_minibatch(client, project, table, b, progress), | ||
| _to_minibatches(data), | ||
| ) | ||
| with tqdm(total=len(data)) as pbar: |
There was a problem hiding this comment.
I think it may be better to have tqdm at a higher level so that its not optional to do progress bar logging. Would it make more sense in materialize_incremental() and then pass it into materialize_single_feature_view?
There was a problem hiding this comment.
I don't see an easy way to bring it up to materialize_incremental since we don't know the number of rows of data at that level; however, progress is an argument in methods in the Provider class so that kinda makes it not optional for implementations of Provider to do some sort of progress logging, presumably by copying existing implementations and using tqdm
There was a problem hiding this comment.
I thought I replied here, but somehow my message didn't send. I think my question is just: How can we ensure consistency in the logging output if the implementations need to create their own tqdm object. Wouldn't the current implementation make it unscalable to have different configurations for tqdm? Even if the row count is deeper, maybe it makes sense to instantiate the tqdm object higher and pass it into the implementation.
|
Nitpicks:
edit: For none of these points I feel super strongly though. Happy to keep as is. |
|
/kind housekeeping |
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
jklegar
force-pushed
the
jacob_materialize_logging
branch
from
aa2757c
to
0ebf141
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jklegar, tsotnet 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:
Approvers can indicate their approval by writing |
* Better logging for materialize command Signed-off-by: Jacob Klegar <jacob@tecton.ai> * Address comments Signed-off-by: Jacob Klegar <jacob@tecton.ai> * Move tqdm to FeatureStore class Signed-off-by: Jacob Klegar <jacob@tecton.ai> * Rebase Signed-off-by: Jacob Klegar <jacob@tecton.ai>
* Better logging for materialize command Signed-off-by: Jacob Klegar <jacob@tecton.ai> * Address comments Signed-off-by: Jacob Klegar <jacob@tecton.ai> * Move tqdm to FeatureStore class Signed-off-by: Jacob Klegar <jacob@tecton.ai> * Rebase Signed-off-by: Jacob Klegar <jacob@tecton.ai>
Signed-off-by: Jacob Klegar jacob@tecton.ai
What this PR does / why we need it: Improve logging for materialize and materialize_incremental
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: