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

[BUG] Array task caching #4731

Closed
2 tasks done
svechinsky opened this issue Jan 16, 2024 · 4 comments
Closed
2 tasks done

[BUG] Array task caching #4731

svechinsky opened this issue Jan 16, 2024 · 4 comments
Assignees
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working exo

Comments

@svechinsky
Copy link

Describe the bug

When running a cacheable array task (classic or array node) if the task includes a custom hash annotation the task won't cache.
This is due to the cache key being generated based on the function name which is generated from the function signature that includes the HashMethod object instantiation.

Expected behavior

The task should cache despite the hash annotation.

Additional context to reproduce

While I haven't tested this you should be able to:

  1. Create a task that returns a cached dataframe (same code as the user guide)
  2. Run it via map task and see that the cache always misses.

Posted here after being asked to do so at the slack channel

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@svechinsky svechinsky added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Jan 16, 2024
Copy link

welcome bot commented Jan 16, 2024

Thank you for opening your first issue here! 🛠

Copy link

dosubot bot commented Jan 16, 2024

🤖

Hello @svechinsky! I'm Dosu, a bot designed to help users like you navigate issues, answer questions and contribute better to our repository. While we wait for the human maintainers to get back to you, I'm here to assist in any way I can. Let's solve some bugs together!

The issue you're experiencing with cache misses when running a cacheable array task with a custom hash annotation might be due to the cache_ignore_input_vars field in the TaskMetadata message. This field is a list of input variables that should not be included when calculating the hash for cache. If the custom hash annotation is included in this list, it would not be considered when calculating the cache key, which could lead to cache misses even when the task should cache.

Here is the relevant part of the code:

// Task Metadata
message TaskMetadata {
    ...
    // cache_ignore_input_vars is the input variables that should not be included when calculating hash for cache.
    repeated string cache_ignore_input_vars  = 13;
}

You can find this in the flyteidl/protos/flyteidl/core/tasks.proto file in the Flyte repository.

Please check your task configuration to ensure that the custom hash annotation is not included in the cache_ignore_input_vars field. If it is, you should remove it from this list to ensure it's considered when calculating the cache key.

As for how the cache key is generated in the Flyte codebase, it is done by converting the data reference into a byte array. This is done using the []byte(reference) operation in Go. The data reference is a unique identifier for the data object in the storage system.

This can be seen in the Head, ReadRaw, WriteRaw, and Delete methods of the cachedRawStore struct in the provided code. For example, in the ReadRaw method:

key := []byte(reference)
if oRaw, err := s.cache.Get(key); err == nil {
	// Found, Cache hit
	s.metrics.CacheHit.Inc()
	return ioutils.NewBytesReadCloser(oRaw), nil
}

Here, key is the cache key, which is generated by converting the reference to a byte array. The reference is passed as an argument to the ReadRaw method. You can find this in the flytestdlib/storage/cached_rawstore.go file in the Flyte repository.

I hope this helps! If you have any other questions or need further clarification, feel free to ask.

Sources


This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@pvditt pvditt self-assigned this Jan 16, 2024
@pvditt pvditt removed the untriaged This issues has not yet been looked at by the Maintainers label Jan 16, 2024
@hamersaw hamersaw added exo backlogged For internal use. Reserved for contributor team workflow. labels Jan 16, 2024
@pvditt
Copy link
Contributor

pvditt commented Jan 16, 2024

@svechinsky thank you for pointing this out. We'll have a fix up for this soon.

@pvditt
Copy link
Contributor

pvditt commented Jan 22, 2024

@svechinsky This should be resolved by: flyteorg/flytekit#2113

One thing to note, this update will cause for cache misses on already cached map_tasks on first runs as the task names are different. Please re-open this issue and tag me if you notice any further issues.

@pvditt pvditt closed this as completed Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlogged For internal use. Reserved for contributor team workflow. bug Something isn't working exo
Projects
None yet
Development

No branches or pull requests

3 participants