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

fix: FIM not caching correctly non-python files #408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aponcedeleonch
Copy link
Contributor

Closes: #405

The implemented cache was not working correctly because of the way the context is added for FIM requests in other programming languages which are not python. The context for the LLM is provided as single line comments. In Python this means lines which start with the character #. Other languages may have other starting sequence for single line comments, e.g. Javascript uses //. This PR changes the regex to detect the paths for other languages.

Closes: #405

The implemented cache was not working correctly because of the
way the context is added for FIM requests in other programming
languages which are not python. The context for the LLM is provided
as single line comments. In Python this means lines which start with
the character `#`. Other languages may have other starting sequence
for single line comments, e.g. Javascript uses `//`. This PR changes
the regex to detect the paths for other languages.
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits here and there but looks good

self._add_cache_entry(hash_key, context)
return True

logger.info(f"FIM entry already in cache: {hash_key}.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be a debug?


def _are_new_alerts_present(self, context: PipelineContext, cached_entry: CachedFim) -> bool:
"""Check if there are new alerts present"""
new_critical_alerts = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this would not alert if there was the same amount of alerts, but different? (This is probably an edge case..)


logger.debug(f"Message to hash: {message_to_hash}")
hashed_content = hashlib.sha256(message_to_hash.encode("utf-8")).hexdigest()
logger.debug(f"Hashed contnet: {hashed_content}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo


# Copilot puts the path at the top of the file. Continue providers contain
# several paths, the one in which the fim is triggered is the last one.
if provider == "copilot":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nicer if we had a provider interface instead of having a list of providers in the DB layer, but that's for later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FIM not caching correctly JS files
2 participants