Skip to content

enable profiling for video-comprehension sample#1958

Closed
alexey-belyakov wants to merge 2 commits into
huggingface:mainfrom
alexey-belyakov:profile_options_for_video_comprehension_example
Closed

enable profiling for video-comprehension sample#1958
alexey-belyakov wants to merge 2 commits into
huggingface:mainfrom
alexey-belyakov:profile_options_for_video_comprehension_example

Conversation

@alexey-belyakov
Copy link
Copy Markdown
Contributor

What does this PR do?

Enable profiling for video-comprehension sample. I needed to collect profiling results for performance degradation issue, to do it

  • I've added profiling arguments as it works in text-generation/run_generation.py
  • To avoid code duplication I've added common_parser.py and moved profiling options to this file.
    Some command lines to test:
    python examples/video-comprehension/run_example.py --help
    python examples/text-generation/run_generation.py --help
    python examples/video-comprehension/run_example.py --model_name_or_path LanguageBind/Video-LLaVA-7B-hf --bf16 --use_hpu_graphs --output_dir /tmp/tmpssf594uz --profiling_steps 10 --profiling_warmup_steps 3 --profiling_record_shapes

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

Comment on lines +44 to +46
project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "../../"))
if project_root not in sys.path:
sys.path.insert(0, project_root)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we need these lines?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

to import this:
from examples.common_parser import add_profiling_args
I do believe it's good approach to use the examples folder for shared code among examples
alternatively:

  • I could use a relative import, but these scripts are not run as a module.
  • I could modify PYTHONPATH, but this would cause changes to all tests and instructions for running these scripts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@regisss can you check my comment please

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.

@alexey-belyakov, I would suggest to wait for #1931 to be merged as there is some rebase of profiling code.

Besides that, probably another solution in case you do not want to rewrite code multiple times is to create a file like examples_utils.py, or the like, under optimum/habana which is already in the path. To avoid the solution you provided which is not very clear.

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.

Honestly I'm also fine in not creating a new file for the three profiling args that are repeating : ) But I can understand the pain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

optimum/habana does not contain code related to examples. examples folder is the most obvious folder for files related to examples.
It is always possible to rename the file after adding more logic to it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that solution is okay. Can you still check if it still holds after #1931 as suggested by @12010486 please?

Comment on lines +36 to +38
project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "../../"))
if project_root not in sys.path:
sys.path.insert(0, project_root)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same

@alexey-belyakov alexey-belyakov requested a review from regisss May 1, 2025 11:07
Comment on lines +44 to +46
project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "../../"))
if project_root not in sys.path:
sys.path.insert(0, project_root)
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.

@alexey-belyakov, I would suggest to wait for #1931 to be merged as there is some rebase of profiling code.

Besides that, probably another solution in case you do not want to rewrite code multiple times is to create a file like examples_utils.py, or the like, under optimum/habana which is already in the path. To avoid the solution you provided which is not very clear.

Comment on lines +44 to +46
project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "../../"))
if project_root not in sys.path:
sys.path.insert(0, project_root)
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.

Honestly I'm also fine in not creating a new file for the three profiling args that are repeating : ) But I can understand the pain

@github-actions
Copy link
Copy Markdown

The code quality check failed, please run make style.

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@astachowiczhabana
Copy link
Copy Markdown
Collaborator

@alexey-belyakov do you still need this change?

@12010486
Copy link
Copy Markdown
Contributor

@astachowiczhabana I believe we can close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants