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

Minor fixes: debug logs, restructuring ifs, fix is_func_in_hunk #192

Merged
merged 2 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion user-deploy-cloud-pat-script.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ spec:
- name: GITHUB_PAT
value: $_GITHUB_PAT
- name: PROVIDER
value: $_PROVIDER
value: $_PROVIDER
- name: LOG_LEVEL
value: Debug
EOF

# Display the generated YAML for debugging
Expand Down
2 changes: 1 addition & 1 deletion vibi-dpu/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "vibi-dpu"
version = "2.0.3"
version = "2.0.4"
edition = "2021"
authors = ["Tapish Rathore <tapish@vibinex.com>"]
license = "GPL-3.0-or-later"
Expand Down
2 changes: 2 additions & 0 deletions vibi-dpu/src/graph/file_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ impl ImportIdentifier {
}
let import_struct_str = import_struct_str_res.expect("Uncaught error in import_struct_str_res");
let prompt_str = format!("{}\nOutput -", &import_struct_str);
log::debug!("[ImportIdentifier/get_import_path] code_chunk: {}", chunk);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid logging sensitive code fragments.

At line 343, the debug log statement outputs the code_chunk, which may contain sensitive code content. Logging code fragments can pose security risks by exposing confidential information in logs. Please consider removing this log statement or ensure that it does not log sensitive data.

let import_path_opt = call_llm_api(prompt_str).await;
// deserialize output
if import_path_opt.is_none() {
Expand All @@ -358,6 +359,7 @@ impl ImportIdentifier {
return None;
}
let import_path: ImportPathOutput = import_path_res.expect("Unacaught error in import_path_res");
log::debug!("[ImportIdentifier/get_import_path] import_path: {:?}", &import_path);
if !import_path.get_matching_import().possible_file_path().is_empty() {
return None;
}
Expand Down
4 changes: 3 additions & 1 deletion vibi-dpu/src/graph/function_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,9 @@ impl FunctionCallIdentifier {
let chunks = numbered_content.chunks(50);
for chunk in chunks {
let chunk_str = chunk.join("\n");
log::debug!("[FunctionCallIdentifier/functions_in_file] chunk = {}", &chunk_str);
if let Some(mut func_calls) = self.functions_in_chunk(&chunk_str, filepath, lang).await {
log::debug!("[FunctionCallIdentifier/functions_in_file] chunk = {:?}", &func_calls);
all_func_calls.function_calls.append(&mut func_calls.function_calls);
}
}
Expand Down Expand Up @@ -346,7 +348,7 @@ impl FunctionCallIdentifier {
hunk_func_pairs.push((hunk.clone(), matching_func_calls_output));
}
}

log::debug!("[FunctionCallIdentifier/function_calls_in_hunks] hunk_func_pairs = {:?}", &hunk_func_pairs);
if hunk_func_pairs.is_empty() {
None
} else {
Expand Down
2 changes: 2 additions & 0 deletions vibi-dpu/src/graph/function_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ impl FunctionNameIdentifier {
}
let prompt_str = prompt_str_res.expect("Uncaught error in prompt_str_res");
let final_prompt = format!("{}\nOutput - ", &prompt_str);
log::debug!("[FunctionNameIdentifier/function_name_in_line] code_line: {}", code_line);
let prompt_response_opt = call_llm_api(final_prompt).await;
if prompt_response_opt.is_none() {
log::error!("[FunctionNameIdentifier/function_name_in_line] Unable to call llm for code line: {:?}", code_line);
Expand All @@ -103,6 +104,7 @@ impl FunctionNameIdentifier {
if func_name.get_function_name().is_empty() {
return None;
}
log::debug!("[FunctionNameIdentifier/function_name_in_line] func_name: {:?}", &func_name);
self.cached_output.insert(code_line.trim().to_string(), func_name.get_function_name().to_string());
return Some(func_name);
}
Expand Down
15 changes: 7 additions & 8 deletions vibi-dpu/src/graph/gitops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,14 @@ impl FileHunks {
&mut self.added_hunks
}

pub fn is_func_in_hunks(&self, function_name: &str) -> &Option<usize> {
for hunk_lines in self.added_hunks() {
if let Some(func_raw) = hunk_lines.function_line() {
if func_raw.contains(function_name) {
return hunk_lines.line_number();
}
}
pub fn is_func_in_hunks(&self, function_name: &str, edge_color: &str) -> &Option<usize> {
let hunks;
if edge_color == "green" {
hunks = self.added_hunks();
} else {
hunks = self.deleted_hunks();
}
for hunk_lines in self.deleted_hunks() {
for hunk_lines in hunks {
Comment on lines +68 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety and return value of is_func_in_hunks method

The changes to the is_func_in_hunks method improve its flexibility by allowing it to check both added and deleted hunks. However, there are some areas for improvement:

  1. Replace the edge_color: &str parameter with an enum for better type safety:
enum HunkType {
    Added,
    Deleted,
}

pub fn is_func_in_hunks(&self, function_name: &str, hunk_type: HunkType) -> Option<(usize, HunkType)> {
    let hunks = match hunk_type {
        HunkType::Added => self.added_hunks(),
        HunkType::Deleted => self.deleted_hunks(),
    };

    // ... rest of the method
}
  1. Update the return type to provide more context about the found hunk:
pub fn is_func_in_hunks(&self, function_name: &str, hunk_type: HunkType) -> Option<(usize, HunkType)> {
    // ... existing logic

    for hunk_lines in hunks {
        if let Some(func_raw) = hunk_lines.function_line() {
            if func_raw.contains(function_name) {
                return hunk_lines.line_number().map(|line| (line, hunk_type));
            }
        }
    }
    None
}

These changes will make the method more type-safe and provide more information to the caller about the found hunk.

if let Some(func_raw) = hunk_lines.function_line() {
if func_raw.contains(function_name) {
return hunk_lines.line_number();
Expand Down
125 changes: 63 additions & 62 deletions vibi-dpu/src/graph/graph_edges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ async fn outgoing_edges(base_filepaths: &Vec<PathBuf>, diff_graph: &DiffGraph,
{
let func_call_identifier_opt = FunctionCallIdentifier::new();
if func_call_identifier_opt.is_none() {
log::error!("[incoming_edges] Unable to create new FunctionCallIdentifier");
log::error!("[outgoing_edges] Unable to create new FunctionCallIdentifier");
return;
}
let mut func_call_identifier = func_call_identifier_opt.expect("Empty func_call_identifier_opt");
Expand Down Expand Up @@ -87,6 +87,11 @@ async fn process_func_calls(import_identifier: &mut ImportIdentifier, func_call_
graph_elems: &mut MermaidGraphElements, edge_color: &str)
{
for (source_filepath, src_file_hunks) in diff_graph.hunk_diff_map().file_line_map() {
let lang_opt = detect_language(source_filepath);
if lang_opt.is_none() {
log::error!("[process_func_calls] Unable to determine language: {}", source_filepath);
continue;
}
let mut source_file_name = source_filepath.to_owned();
// get func calls
if let Some(source_file) = absolute_to_relative_path(source_filepath, review) {
Expand All @@ -98,34 +103,65 @@ async fn process_func_calls(import_identifier: &mut ImportIdentifier, func_call_
} else {
diff_hunks = src_file_hunks.deleted_hunks();
}
let lang_opt = detect_language(source_filepath);
if lang_opt.is_none() {
log::error!("[get_import_path_file] Unable to determine language: {}", source_filepath);
return;
}
log::debug!("[process_func_calls] file name: {}\n, diff_hunks: {:?}, edge: {}", &source_file_name, diff_hunks, edge_color);
let lang = lang_opt.expect("Empty lang_opt");
let source_file_path = Path::new(source_filepath);
let source_file_pathbuf = source_file_path.to_path_buf();
if let Some(hunk_func_calls) = func_call_identifier.
function_calls_in_hunks(&source_file_pathbuf, &lang, diff_hunks).await {
for (hunk_lines, func_call_output) in hunk_func_calls {
for dest_func_call in func_call_output.function_calls() {
if let Some(import_filepath) = import_identifier.get_import_path_file(
source_filepath, &lang, dest_func_call.function_name()).await {
// get file
// get diffgraph all files and see if they contain filepath
let possible_diff_file_paths: Vec<&String> = diff_graph.hunk_diff_map().all_files().into_iter()
.filter(|file_path| file_path.contains(import_filepath.get_matching_import().possible_file_path())).collect();
if !possible_diff_file_paths.is_empty() {
for possible_diff_file_path in possible_diff_file_paths {
if diff_graph.hunk_diff_map().all_files().contains(&possible_diff_file_path)
{
let hunks_for_func = diff_graph.hunk_diff_map().file_line_map()
.get(possible_diff_file_path).expect("Empty entry in file_line_map");
if let Some(possible_file_rel) = absolute_to_relative_path(possible_diff_file_path, review) {
if let Some(dest_func_def_line) = hunks_for_func.is_func_in_hunks(dest_func_call.function_name()) {
if let Some(src_func_name) = hunk_lines.function_line() {
if let Some(src_func_line_number) = hunk_lines.line_number() {
if let Some(src_func_name) = hunk_lines.function_line() {
if let Some(src_func_line_number) = hunk_lines.line_number() {
for dest_func_call in func_call_output.function_calls() {
if let Some(import_filepath) = import_identifier.get_import_path_file(
source_filepath, &lang, dest_func_call.function_name()).await {
// get file
// get diffgraph all files and see if they contain filepath
let possible_diff_file_paths: Vec<&String> = diff_graph.hunk_diff_map().all_files().into_iter()
.filter(|file_path| file_path.contains(import_filepath.get_matching_import().possible_file_path())).collect();
log::debug!("[process_func_calls] possible_diff_file_paths = {:?}", &possible_diff_file_paths);
if !possible_diff_file_paths.is_empty() {
for possible_diff_file_path in possible_diff_file_paths {
if diff_graph.hunk_diff_map().all_files().contains(&possible_diff_file_path)
{
log::debug!("[process_func_calls] possible_diff_file_path ={}", &possible_diff_file_path);
Comment on lines +125 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify conditional check

The condition in line 125 checks if possible_diff_file_path is contained within diff_graph.hunk_diff_map().all_files(). Since possible_diff_file_paths is already filtered based on this condition, this check may be redundant.

Consider removing the redundant condition to streamline the code.

if let Some(possible_file_rel) = absolute_to_relative_path(possible_diff_file_path, review) {
let hunks_for_func = diff_graph.hunk_diff_map().file_line_map()
.get(possible_diff_file_path).expect("Empty entry in file_line_map");
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid potential panics by handling Option properly

Using expect("Empty entry in file_line_map") at line 130 can cause a panic if the entry does not exist. It's safer to handle the None case to prevent unexpected crashes.

Apply this diff to handle the None case:

-let hunks_for_func = diff_graph.hunk_diff_map().file_line_map()
-    .get(possible_diff_file_path).expect("Empty entry in file_line_map");
+if let Some(hunks_for_func) = diff_graph.hunk_diff_map().file_line_map()
+    .get(possible_diff_file_path) {
+    // Existing code that uses hunks_for_func
+} else {
+    log::error!("[process_func_calls] No entry found in file_line_map for {}", possible_diff_file_path);
+    continue;
+}

Committable suggestion was skipped due to low confidence.

if let Some(dest_func_def_line) = hunks_for_func.is_func_in_hunks(dest_func_call.function_name(), edge_color) {
graph_elems.add_edge(
edge_color,
dest_func_call.line_number().to_owned() as usize,
src_func_name,
dest_func_call.function_name(),
&source_file_name,
&possible_file_rel,
"yellow",
"",
src_func_line_number,
dest_func_def_line);
}
}
}
}
} else {
// search all files
// TODO - see if git checkout is needed
let possible_file_pathbufs: Vec<&PathBuf> = base_filepaths.iter()
.filter(|file_path|
file_path.to_string_lossy().contains(import_filepath.get_matching_import().possible_file_path())).collect();
log::debug!("[process_func_calls] possible_file_pathbufs = {:?}", &possible_file_pathbufs);
Comment on lines +150 to +153
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use path comparison methods instead of string matching

In lines 150-153, paths are compared using to_string_lossy() and contains(), which may lead to incorrect behavior on different platforms or with complex paths. It's better to use path operations for reliable comparisons.

Apply this diff to compare paths more robustly:

-let possible_file_pathbufs: Vec<&PathBuf> = base_filepaths.iter()
-    .filter(|file_path| 
-        file_path.to_string_lossy().contains(import_filepath.get_matching_import().possible_file_path())).collect();
+let import_path = Path::new(import_filepath.get_matching_import().possible_file_path());
+let possible_file_pathbufs: Vec<&PathBuf> = base_filepaths.iter()
+    .filter(|file_path| file_path.ends_with(import_path)).collect();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let possible_file_pathbufs: Vec<&PathBuf> = base_filepaths.iter()
.filter(|file_path|
file_path.to_string_lossy().contains(import_filepath.get_matching_import().possible_file_path())).collect();
log::debug!("[process_func_calls] possible_file_pathbufs = {:?}", &possible_file_pathbufs);
let import_path = Path::new(import_filepath.get_matching_import().possible_file_path());
let possible_file_pathbufs: Vec<&PathBuf> = base_filepaths.iter()
.filter(|file_path| file_path.ends_with(import_path)).collect();
log::debug!("[process_func_calls] possible_file_pathbufs = {:?}", &possible_file_pathbufs);

if !possible_file_pathbufs.is_empty() {
for possible_file_pathbuf in possible_file_pathbufs {
let possible_file_path: String = possible_file_pathbuf.to_string_lossy().to_string();
if let Some(possible_file_rel) =
absolute_to_relative_path(&possible_file_path, review) {
// search only for func def with specific name
// if something comes up, add edge!
if let Some(func_def) = funcdef_identifier.function_defs_in_file(
possible_file_pathbuf, &lang, dest_func_call.function_name()).await {
log::debug!("[process_func_calls] func_def ={:#?}", &func_def);
if let Some(dest_func_def_line) = func_def.get_function_line_number() {
Comment on lines +161 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Check for potential performance improvements

At lines 161-164, when searching for function definitions, consider caching results or optimizing the search if this operation is frequent and affects performance.

Would you like to explore strategies for optimizing function definition searches?

graph_elems.add_edge(
edge_color,
dest_func_call.line_number().to_owned() as usize,
Expand All @@ -136,51 +172,16 @@ async fn process_func_calls(import_identifier: &mut ImportIdentifier, func_call_
"yellow",
"",
src_func_line_number,
dest_func_def_line);
}
}
}
}
}
}
} else {
// search all files
// TODO - see if git checkout is needed
let possible_file_pathbufs: Vec<&PathBuf> = base_filepaths.iter()
.filter(|file_path|
file_path.to_string_lossy().contains(import_filepath.get_matching_import().possible_file_path())).collect();
if !possible_file_pathbufs.is_empty() {
for possible_file_pathbuf in possible_file_pathbufs {
let possible_file_path: String = possible_file_pathbuf.to_string_lossy().to_string();
// search only for func def with specific name
// if something comes up, add edge!
if let Some(func_defs) = funcdef_identifier.function_defs_in_file(
possible_file_pathbuf, &lang, dest_func_call.function_name()).await {
if let Some(dest_func_def_line) = func_defs.get_function_line_number() {
if let Some(src_func_name) = hunk_lines.function_line() {
if let Some(src_func_line_number) = hunk_lines.line_number() {
if let Some(possible_file_rel) =
absolute_to_relative_path(&possible_file_path, review) {
graph_elems.add_edge(
edge_color,
dest_func_call.line_number().to_owned() as usize,
src_func_name,
dest_func_call.function_name(),
&source_file_name,
&possible_file_rel,
"yellow",
"",
src_func_line_number,
&dest_func_def_line);
}
&dest_func_def_line);
}
}
}
}
}
}
}
}
}
}
}
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions vibi-dpu/src/graph/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ struct LlmResponse {
pub async fn call_llm_api(prompt: String) -> Option<String> {
let client = get_client();
let url = "http://34.100.208.132/api/generate";
log::debug!("[call_llm_api] Prompt = {:?}", &prompt);
let response_res = client.post(url)
.json(&json!({"model": "phind-codellama", "prompt": prompt}))
.send()
Expand Down Expand Up @@ -76,11 +75,11 @@ pub async fn call_llm_api(prompt: String) -> Option<String> {
}
}
}
log::debug!("[call_llm_api] final response = {}", &final_response);
let final_response_trimmed = final_response.trim();
if final_response_trimmed.starts_with("{") && !final_response_trimmed.ends_with("}") {
final_response.push_str("}");
}
log::debug!("[call_llm_api] final_response = {:?}", &final_response);
Some(final_response)
}

Expand Down
Loading