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

Improve examples #3091

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

AlexanderDokuchaev
Copy link
Collaborator

@AlexanderDokuchaev AlexanderDokuchaev commented Nov 15, 2024

Changes

  • Add pattern based on file extension to gitignore for examples
  • Use pathlib style instead os.path
  • Fix issue with creating ../nncf/~/.cache/datasets directory
  • Replace tqdm to nncf.common.logging.track_progress
  • Remove unused function arguments and if conditions
  • Clean logs, disable progress bar, remove log of validataion
  • Pass list of command arguments to subprocess instead of string
  • Remove nncf dependencies from tests/cross_fw/examples
  • Add --reuse-venv to use venv from example directory
  • Add progress bar to collect_values_for_each_item_using_prepared_model

Tests

https://github.com/openvinotoolkit/nncf/actions/runs/11900403097
nightly/job/test_examples/618
nightly/job/windows/job/test-examples/278/

@github-actions github-actions bot added NNCF TF Pull requests that updates NNCF TensorFlow NNCF PT Pull requests that updates NNCF PyTorch NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF ONNX Pull requests that updates NNCF ONNX labels Nov 15, 2024
@AlexanderDokuchaev AlexanderDokuchaev changed the title examples Improve examples Nov 18, 2024
@AlexanderDokuchaev AlexanderDokuchaev marked this pull request as ready for review November 18, 2024 20:53
@AlexanderDokuchaev AlexanderDokuchaev requested a review from a team as a code owner November 18, 2024 20:53
@MaximProshin
Copy link
Collaborator

@AlexanderDokuchaev , @alexsu52 , please run SDL checks before the merge (as you changed some imports and the way how 'path' is used). Also make sure it works on Windows.

@github-actions github-actions bot added the NNCF PTQ Pull requests that updates NNCF PTQ label Nov 19, 2024
@AlexanderDokuchaev
Copy link
Collaborator Author

@MaximProshin
/sdl/job/nightly_trigger/346 - pass, exclude checkmarkx: warning about .github/..
nightly/job/windows/job/test-examples/278/

.github/workflows/examples.yml Show resolved Hide resolved
examples/llm_compression/openvino/tiny_llama_find_hyperparams/statistics
compressed_graph.dot
original_graph.dot
datasets/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why datasets/** was removed?
In case of the running tests locally, datasets directory may appear and be unintentionally committed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

datasets directory created by ultralitics, it's only happens if you have yolo config .config/Ultralytics/settings.json file that was generated some time ago or changes manually.

On current version of yolo, default path for datasets_dir generated as nncf/../datasest if run example from NNCF in case of .config/Ultralytics/settings.json is not exist.
If datasets directory created on your host you need to modify config by yolo settings dataset_dir=new_path or by text editor, or remove config file after it will be generated this new path.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not agree.
I've run the following command from the nncf root (as usual, for testing purposes):
pytest -s tests/cross_fw/examples/test_examples.py -k "post_training_quantization_openvino_yolo8_quantize"
And after the passing, I observe the following directory:
image
Thus, removing datasets/** from the .gitignore might cause issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From a high perspective, it would be more helpful if you would update examples to create a datasets folder during the run in a corresponding or temporary directory, to exclude incidents in the future. In this case, we would be able to remove this path from .gitignore and safely run tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have you updated the ultralytics config file as described above?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. I did nothing with the ultralytics package. I've reproduced simple scenario with the following steps:

  1. Create clean virtual env;
  2. Clone nncf from GitHub;
  3. Install nncf & pytest;
  4. Run test;

From the contributor's perspective, I would not even know anything about ultralytics managing at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You have config file ~/.config/Ultralytics/settings.json on your host that was generated when you run any code with ultralitics for the first time and creating or removing any virtual environment does not affect on the config in any way.

Your config collect line like "datasets_dir": "/home/<username>/nncf/datasets",.
This line only generated if run NNCF examples with old version of ultralitics, current default value is like /home/<username>/datasets - GIT_ROOT.parent / "datasets".

There is only one way conditions to get datasets_dir like .../nncf/datasets:

  • You did not run any code with ultralitics before
  • And for the first time you have run NNCF yolo example with old version of ultralitics

In all other cases datasets_dir will not set as nncf/datasets

My point, If you have config file of any third party package with not default values in your ~/.config directory, that used NNCF folder as storage for anything, it's not responsibility of NNCF, it's incorrect configuration that should be resolved by user.

Moreover, the datasets directory in the NNCF git root folder breaks isort, by changing the import order of datasets module.
So datasets in .gitignore hides this problem by making the existence of this directory expected but it is not.

@alexsu52 as you have added datasets to .gitignore, i think last word is your

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not responsibility of NNCF, it's incorrect configuration that should be resolved by user.

If I run NNCF tests from the root and NNCF creates a datasets folder in the root, this is NNCF's responsibility, not the user, since it is not possible to set up the folder for this via tests.
As you said,

datasets directory in the NNCF git root folder breaks isort

So, this is another reason to change the placing of the datasets folder on the NNCF's side, since tests were designed by the NNCF team.

Copy link
Contributor

Choose a reason for hiding this comment

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

I read your discussion and did the following experiment:

  • remove ~/.config/Ultralytics/settings.json
  • create a clear env using
pip install -r examples/post_training_quantization/onnx/yolov8_quantize_with_accuracy_control/requirements.txt

Ultralytics version:
image

  • run example
python examples/post_training_quantization/onnx/yolov8_quantize_with_accuracy_control/main.py

dataset was downloaded into nncf/datasets
image

Where did I go wrong?

tests/cross_fw/shared/command.py Show resolved Hide resolved
tests/cross_fw/shared/helpers.py Show resolved Hide resolved
@KodiaqQ
Copy link
Collaborator

KodiaqQ commented Nov 20, 2024

@AlexanderDokuchaev, feel free to merge after approval from the other reviewers.

@KodiaqQ KodiaqQ dismissed their stale review November 20, 2024 16:19

Not blocking merge.

@KodiaqQ KodiaqQ removed their request for review November 20, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NNCF ONNX Pull requests that updates NNCF ONNX NNCF OpenVINO Pull requests that updates NNCF OpenVINO NNCF PT Pull requests that updates NNCF PyTorch NNCF PTQ Pull requests that updates NNCF PTQ NNCF TF Pull requests that updates NNCF TensorFlow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants