Skip to content
This repository was archived by the owner on Apr 23, 2026. It is now read-only.

Add test to track config file changes - #1683

Merged
mergify[bot] merged 1 commit into
instructlab:mainfrom
tiran:default-config-test
Jul 24, 2024
Merged

Add test to track config file changes#1683
mergify[bot] merged 1 commit into
instructlab:mainfrom
tiran:default-config-test

Conversation

@tiran

@tiran tiran commented Jul 12, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

A new test compares the config from get_default_config against a default config file in VCS. This allows us to track any change in the git history.

See: #1725

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Changelog updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@mergify mergify Bot added testing Relates to testing ci-failure PR has at least one CI failure labels Jul 12, 2024
@tiran
tiran force-pushed the default-config-test branch from 992e81d to 6733785 Compare July 12, 2024 10:26
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jul 12, 2024
@tiran
tiran requested review from leseb and russellb July 12, 2024 15:17
@tiran
tiran force-pushed the default-config-test branch from 6733785 to 698940e Compare July 13, 2024 08:28
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jul 13, 2024
leseb
leseb reviewed Jul 15, 2024
View reviewed changes
Comment thread tests/test_config.py Outdated
Comment thread tests/test_config.py Outdated
@tiran
tiran force-pushed the default-config-test branch from 698940e to 7c4319d Compare July 15, 2024 10:29
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jul 15, 2024
@tiran
tiran force-pushed the default-config-test branch from 7c4319d to 4ec7f69 Compare July 16, 2024 09:30
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jul 16, 2024
@tiran
tiran force-pushed the default-config-test branch from 4ec7f69 to 5916ef4 Compare July 17, 2024 06:29
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jul 17, 2024
@tiran
tiran force-pushed the default-config-test branch from 5916ef4 to f7a5734 Compare July 17, 2024 06:44
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jul 17, 2024
@tiran
tiran requested a review from leseb July 17, 2024 07:29
leseb
leseb reviewed Jul 17, 2024
View reviewed changes

@leseb leseb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks all good to me. However, I'm wondering if tests/testdata/default_config.yaml should be located elsewhere to gain more visibility. If so, how about renaming it to sample.ilab.config.yaml or sample.config.yaml to indicate that it's a sample configuration file? And have it at the root? or in src? Thanks!

@leseb

leseb commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

Pending from offline discussion, we need to make it clearer that when the test fails users have to run make regenerate-testdata and commit the change.

@tiran
tiran force-pushed the default-config-test branch from f7a5734 to 9a8302b Compare July 17, 2024 13:45
leseb
leseb reviewed Jul 17, 2024
View reviewed changes
Comment thread tests/test_config.py Outdated
@tiran
tiran force-pushed the default-config-test branch from 9a8302b to 7e949c7 Compare July 17, 2024 14:00
@mergify mergify Bot added ci-failure PR has at least one CI failure and removed ci-failure PR has at least one CI failure labels Jul 17, 2024
@leseb

leseb commented Jul 18, 2024

Copy link
Copy Markdown
Contributor

This looks all good to me. However, I'm wondering if tests/testdata/default_config.yaml should be located elsewhere to gain more visibility. If so, how about renaming it to sample.ilab.config.yaml or sample.config.yaml to indicate that it's a sample configuration file? And have it at the root? or in src? Thanks!

Discussed offline, we don't want to mix test_data with this proposal, so we will do separately.

@mergify mergify Bot added the one-approval PR has one approval from a maintainer label Jul 18, 2024
@tiran tiran mentioned this pull request Jul 18, 2024
Merged
5 tasks
@mergify mergify Bot removed the one-approval PR has one approval from a maintainer label Jul 22, 2024
@leseb

leseb commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

@Mergifyio rebase

@mergify

mergify Bot commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

rebase

✅ Branch has been successfully rebased

@leseb
leseb force-pushed the default-config-test branch from 7e949c7 to 78617f8 Compare July 22, 2024 14:27
@mergify mergify Bot added the ci-failure PR has at least one CI failure label Jul 22, 2024
@leseb

leseb commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

CI is failing as expected :), I rebased on top of the version flag addition.

  E       AssertionError: current and expected configs are different. If the change was intentional, run 'make regenerate-testdata' and commit the updated test data.
  E       assert {'chat': {'context': 'default', 'greedy_mode': False, 'logs_dir': '/data/instructlab/chatlogs', 'max_tokens': None, 'model': '/data/instructlab/models/merlinite-7b-lab-Q4_K_M.gguf', 'session': None, 'vi_mode': False, 'visible_overflow': True}, 'evaluate': {'base_branch': None, 'base_model': 'instructlab/granite-7b-lab', 'branch': None, 'gpus': 1, 'mmlu': {'batch_size': 'auto', 'few_shots': 2}, 'mmlu_branch': {'tasks_dir': '/data/instructlab/datasets'}, 'model': None, 'mt_bench': {'judge_model': 'prometheus-eval/prometheus-8x7b-v2.0', 'max_workers': 16, 'output_dir': '/data/instructlab/internal/eval_data'}, 'mt_bench_branch': {'taxonomy_path': '/data/instructlab/taxonomy'}}, 'general': {'debug_level': 0, 'log_level': 'INFO'}, 'generate': {'chunk_word_count': 1000, 'model': '/data/instructlab/models/merlinite-7b-lab-Q4_K_M.gguf', 'num_cpus': 10, 'output_dir': '/data/instructlab/datasets', 'prompt_file': '/data/instructlab/internal/prompt.txt', 'sdg_scale_factor': 30, 'seed_file': '/data/instructlab/internal/seed_tasks.json', 'taxonomy_base': 'origin/main', 'taxonomy_path': '/data/instructlab/taxonomy'}, 'serve': {'backend': None, 'chat_template': None, 'host_port': '127.0.0.1:8000', 'llama_cpp': {'gpu_layers': -1, 'llm_family': '', 'max_ctx_size': 4096}, 'model_path': '/data/instructlab/models/merlinite-7b-lab-Q4_K_M.gguf', 'vllm': {'llm_family': '', 'vllm_args': []}}, 'train': {'additional_args': {}, 'ckpt_output_dir': '/cache/instructlab/checkpoints', 'data_output_dir': '/data/instructlab/internal', 'data_path': '/data/instructlab/datasets', 'deepspeed_cpu_offload_optimizer': False, 'effective_batch_size': 3840, 'is_padding_free': False, 'lora_quantize_dtype': 'nf4', 'lora_rank': 4, 'max_batch_len': 10000, 'max_seq_len': 4096, 'model_path': 'instructlab/granite-7b-lab', 'nproc_per_node': 1, 'num_epochs': 10, 'save_samples': 250000}, 'version': '1.0.0'} == {'chat': {'context': 'default', 'greedy_mode': False, 'logs_dir': '/data/instructlab/chatlogs', 'max_tokens': None, 'model': '/data/instructlab/models/merlinite-7b-lab-Q4_K_M', 'session': None, 'vi_mode': False, 'visible_overflow': True}, 'evaluate': {'base_branch': None, 'base_model': 'instructlab/granite-7b-lab', 'branch': None, 'gpus': 1, 'mmlu': {'batch_size': 'auto', 'few_shots': 2}, 'mmlu_branch': {'tasks_dir': '/data/instructlab/datasets'}, 'model': None, 'mt_bench': {'judge_model': 'prometheus-eval/prometheus-8x7b-v2.0', 'max_workers': 16, 'output_dir': '/data/instructlab/internal/eval_data'}, 'mt_bench_branch': {'taxonomy_path': '/data/instructlab/taxonomy'}}, 'general': {'debug_level': 0, 'log_level': 'INFO'}, 'generate': {'chunk_word_count': 1000, 'model': '/data/instructlab/models/merlinite-7b-lab-Q4_K_M', 'num_cpus': 10, 'num_instructions': 100, 'output_dir': '/data/instructlab/datasets', 'prompt_file': '/data/instructlab/internal/prompt.txt', 'seed_file': '/data/instructlab/internal/seed_tasks.json', 'taxonomy_base': 'origin/main', 'taxonomy_path': '/data/instructlab/taxonomy'}, 'serve': {'backend': None, 'chat_template': None, 'host_port': '127.0.0.1:8000', 'llama_cpp': {'gpu_layers': -1, 'llm_family': '', 'max_ctx_size': 4096}, 'model_path': '/data/instructlab/models/merlinite-7b-lab-Q4_K_M', 'vllm': {'llm_family': '', 'vllm_args': []}}, 'train': {'additional_args': {}, 'ckpt_output_dir': '/cache/instructlab/checkpoints', 'data_output_dir': '/data/instructlab/internal', 'data_path': '/data/instructlab/datasets', 'deepspeed_cpu_offload_optimizer': False, 'effective_batch_size': 3840, 'is_padding_free': False, 'lora_quantize_dtype': 'nf4', 'lora_rank': 4, 'max_batch_len': 10000, 'max_seq_len': 4096, 'model_path': 'instructlab/granite-7b-lab', 'nproc_per_node': 1, 'num_epochs': 10, 'save_samples': 250000}}

@tiran
Add test to track config file changes
2d3d670 
A new test compares the config from `get_default_config` against a
default config file in VCS. This allows us to track any change in the
git history.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
tiran force-pushed the default-config-test branch from 78617f8 to 2d3d670 Compare July 24, 2024 06:22
@mergify mergify Bot removed the ci-failure PR has at least one CI failure label Jul 24, 2024
@mergify
mergify Bot merged commit 24acebd into instructlab:main Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@leseb leseb leseb approved these changes
@nathan-weinberg nathan-weinberg nathan-weinberg approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testing Relates to testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tiran @leseb @nathan-weinberg