Skip to content

Build settings index in parallel for the native server#12299

Merged
dhruvmanila merged 2 commits intomainfrom
dhruv/parallel-walk-builder
Jul 15, 2024
Merged

Build settings index in parallel for the native server#12299
dhruvmanila merged 2 commits intomainfrom
dhruv/parallel-walk-builder

Conversation

@dhruvmanila
Copy link
Copy Markdown
Member

@dhruvmanila dhruvmanila commented Jul 12, 2024 •
edited
Loading

Summary

This PR updates the server to build the settings index in parallel using similar logic as python_files_in_path.

This should help with #11366 but ideally we would want to build it lazily.

Test Plan

cargo insta test

@dhruvmanila dhruvmanila added the server Related to the LSP server label Jul 12, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 12, 2024 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error) 

warning: Detected debug build without --no-cache.
error: Failed to parse examples/chatgpt/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/chatgpt/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error) 

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to parse examples/chatgpt/gpt_actions_library/.gpt_action_getting_started.ipynb:11:1:1: Expected an expression
error: Failed to parse examples/chatgpt/gpt_actions_library/gpt_action_bigquery.ipynb:13:1:1: Expected an expression

MichaReiser
MichaReiser approved these changes Jul 12, 2024
View reviewed changes
Comment thread crates/ruff_server/src/session/index/ruff_settings.rs Outdated

let directory = entry.into_path();
let mut builder = WalkBuilder::new(root);
builder.standard_filters(true);
Copy link
Copy Markdown
Member

@MichaReiser MichaReiser Jul 12, 2024

Choose a reason for hiding this comment

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

Do we need to respect the respect_gitignore setting here?

Copy link
Copy Markdown
Member Author

@dhruvmanila dhruvmanila Jul 12, 2024

Choose a reason for hiding this comment

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

I think so. Let me check.

Copy link
Copy Markdown
Member Author

@dhruvmanila dhruvmanila Jul 15, 2024

Choose a reason for hiding this comment

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

I've added it where by default it's true and if we find any config file it'll use the value from that instead.

@MichaReiser
Copy link
Copy Markdown
Member

MichaReiser commented Jul 12, 2024

What's also important to note is that this will exclude directories that are ignored

@charliermarsh
Copy link
Copy Markdown
Member

charliermarsh commented Jul 12, 2024

Nice.

@dhruvmanila
Copy link
Copy Markdown
Member Author

dhruvmanila commented Jul 15, 2024

What's also important to note is that this will exclude directories that are ignored

I don't think that's the case if there's no config file present in the root directory. This is because the first loop extracts the settings from the root config file:

ruff/crates/ruff_server/src/session/index/ruff_settings.rs

Lines 105 to 128 in ecd6865

for directory in root.ancestors() {
if let Some(pyproject) = settings_toml(directory).ok().flatten() {
let Ok(settings) = ruff_workspace::resolver::resolve_root_settings(
&pyproject,
Relativity::Parent,
&EditorConfigurationTransformer(editor_settings, root),
) else {
continue;
};
index.insert(
directory.to_path_buf(),
Arc::new(RuffSettings {
path: Some(pyproject),
file_resolver: settings.file_resolver,
linter: settings.linter,
formatter: settings.formatter,
}),
);
break;
}
}
// Add any settings within the workspace itself

This is then used when walking through the project files to determine whether a directory should be excluded or not:

ruff/crates/ruff_server/src/session/index/ruff_settings.rs

Lines 141 to 205 in ecd6865

Box::new(|result| {
let Ok(entry) = result else {
return WalkState::Continue;
};
// Skip non-directories.
if !entry
.file_type()
.is_some_and(|file_type| file_type.is_dir())
{
return WalkState::Continue;
}
let directory = entry.into_path();
tracing::debug!("Visiting: {}", directory.display());
// If the directory is excluded from the workspace, skip it.
if let Some(file_name) = directory.file_name() {
if let Some((_, settings)) = index
.read()
.unwrap()
.range(..directory.clone())
.rfind(|(path, _)| directory.starts_with(path))
{
if match_exclusion(&directory, file_name, &settings.file_resolver.exclude) {
tracing::debug!("Ignored path via `exclude`: {}", directory.display());
return WalkState::Continue;
} else if match_exclusion(
&directory,
file_name,
&settings.file_resolver.extend_exclude,
) {
tracing::debug!(
"Ignored path via `extend-exclude`: {}",
directory.display()
);
return WalkState::Continue;
}
}
}
if let Some(pyproject) = settings_toml(&directory).ok().flatten() {
let Ok(settings) = ruff_workspace::resolver::resolve_root_settings(
&pyproject,
Relativity::Parent,
&EditorConfigurationTransformer(editor_settings, root),
) else {
return WalkState::Continue;
};
index.write().unwrap().insert(
directory,
Arc::new(RuffSettings {
path: Some(pyproject),
file_resolver: settings.file_resolver,
linter: settings.linter,
formatter: settings.formatter,
}),
);
}
WalkState::Continue
})
});
let fallback = Arc::new(RuffSettings::fallback(editor_settings, root));

Maybe we can use the fallback settings if there is none in the root directory?

ruff/crates/ruff_server/src/session/index/ruff_settings.rs

Line 205 in ecd6865

let fallback = Arc::new(RuffSettings::fallback(editor_settings, root));

@dhruvmanila dhruvmanila force-pushed the dhruv/parallel-walk-builder branch from cb87b15 to ba08519 Compare July 15, 2024 09:53
@dhruvmanila dhruvmanila enabled auto-merge (squash) July 15, 2024 09:53
@dhruvmanila dhruvmanila merged commit ecd4b4d into main Jul 15, 2024
@dhruvmanila dhruvmanila deleted the dhruv/parallel-walk-builder branch July 15, 2024 09:57
@dhruvmanila dhruvmanila mentioned this pull request Jul 17, 2024
Merged
dhruvmanila added a commit that referenced this pull request Jul 17, 2024
@dhruvmanila
Remove leftover debug log (#12360)
6e0cbe0 
This was a leftover from #12299
@dhruvmanila dhruvmanila mentioned this pull request Jul 17, 2024
Merged
@BrewTestBot BrewTestBot mentioned this pull request Jul 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees

No one assigned

Labels

server Related to the LSP server

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dhruvmanila @MichaReiser @charliermarsh