⚗️ try swapping to ruff and precommit #532
Conversation
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
|
pre-commit seems to work slick! The diff from actually swapping here is pretty hefty tho |
pyproject.toml
Outdated
| # Allow lines to be as long as 80. | ||
| line-length = 80 | ||
| # Use widescreen monitors 😎😎😎 | ||
| line-length = 120 |
There was a problem hiding this comment.
not sure how that'll look with folks using laptop only :)
There was a problem hiding this comment.
lol yeah I wanted to see what the negative diff here is and it's like 600 lines of savings
It seems pretty fine on my laptop screen, but let me know what you think!
There was a problem hiding this comment.
I like 120 for my local dev, but when looking at GitHub PR diffs side-by-side, the 80 character line limit seems to work better. With 120 character, Github needs to wrap those lines and then Python's semantic indentation gets lost and it's harder to track context when reading the code changes.
ckadner
left a comment
ckadner
left a comment
There was a problem hiding this comment.
Overall this seems to be a positive change. Maybe we can tweak some of our code conventions a little bit and allow a bit more freedom in favor of code readability in some places.
tests/aftu/test_compare_graphs.py
Outdated
| str(max_num_seqs), | ||
| "--compile_dynamic_sendnn", | ||
| "--attention_type", | ||
| attn_type, |
There was a problem hiding this comment.
This does not occur often, but in cases like this one I would favor readability over consistency and compactness (or line width) to group parameter names and values on one line (if the code formatting police allows it)
inference_py_args = [
sys.executable, "scripts/inference.py",
"--architecture", "hf_pretrained",
"--model_path", model_path,
"--tokenizer", model_path,
"--unfuse_weights",
"--device_type", "aiu",
"--compile",
"--cast_bf16_to_fp16",
"--compile_dynamic",
"--min_pad_length", "64",
"--max_new_tokens", "5",
"--batch_size", str(max_num_seqs),
"--compile_dynamic_sendnn",
"--attention_type", attn_typeThere was a problem hiding this comment.
We'd probably have to add inline comments that turn off the formatting for a specific block, but yeah I agree that what you posted here is much more readable than either the pre or post diff here
| ignore_eos=True) | ||
| params0 = SamplingParams(max_tokens=5, temperature=0, logprobs=0, ignore_eos=True) | ||
| params1 = SamplingParams(max_tokens=10, temperature=0, logprobs=0, ignore_eos=True) | ||
| params2 = SamplingParams(max_tokens=7, temperature=0, logprobs=0, ignore_eos=True) |
There was a problem hiding this comment.
👍🏻 this is better than before
| sampling_params = sampling_params_1k + sampling_params_2k + \ | ||
| sampling_params_4k + sampling_params_8k + sampling_params_16k + \ | ||
| sampling_params_32k | ||
| sampling_params = ( |
pyproject.toml
Outdated
| # Allow lines to be as long as 80. | ||
| line-length = 80 | ||
| # Use widescreen monitors 😎😎😎 | ||
| line-length = 120 |
There was a problem hiding this comment.
Here is an example of readability getting worse at least when looking at Github diffs:
or here
|
I agree with @ckadner's assessment. Overall it's positive, but 120 character lines seem to make diffs more difficult to read. |
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]> # Conflicts: # tests/aftu/graph_compare_utils.py # tests/aftu/test_compare_graphs.py # tests/e2e/test_sampling_params.py # tests/golden_token_injector.py # tests/llm_cache.py # tests/output_util.py # tests/spyre_util.py # tests/utils/test_model_config_validator.py # tests/v1/worker/test_spyre_input_batch.py # vllm_spyre/config/runtime_config_validator.py # vllm_spyre/envs.py # vllm_spyre/platform.py # vllm_spyre/v1/sample/spyre_logits_processor.py # vllm_spyre/v1/worker/spyre_input_batch.py # vllm_spyre/v1/worker/spyre_worker.py
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
|
Seems to work with |
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Description
Popping in vllm's current pre-config setup, seeing how easy it'd be to replace our current setup with this
Update:
Using
prekinstead of pre-commit (prekis a much faster Rust based swap-in for pre-commit)