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

Add Multimodal heading and Document question answering in task_summary.mdx #23318

Merged

Conversation

y3sar
Copy link
Contributor

@y3sar y3sar commented May 12, 2023

What does this PR do?

From #18926
This PR creates a new Multimodal heading in task_summary and add Document question answering task example inside of it.

Who can review?

@stevhliu

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 12, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Really nice - thanks for adding!

@amyeroberts
Copy link
Collaborator

@y3sar - The current CI tests are failing because this PR triggers tests checking code snippets in the docs, and the doctest CI environment doesn't have ffmpeg installed. I've opened a PR #23329 to resolve this. Once I've confirmed everything works, we can re-run for this PR and if all green then merge :)

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Looks great, thanks again for your contribution and for adding the new multimodal section! 🤗

@amyeroberts
Copy link
Collaborator

@y3sar Sorry for the delay in this. There's been a lot of changes in how doctests are retrieved and run. Could you rebase onto main to get the latest updates?

@y3sar
Copy link
Contributor Author

y3sar commented Jun 11, 2023

@amyeroberts sure thing I'll rebase and commit again

@amyeroberts
Copy link
Collaborator

@y3sar Did you force push after rebasing the branch? The current commit history looks like what I get if I push but don't force.

@y3sar
Copy link
Contributor Author

y3sar commented Jun 12, 2023

@amyeroberts no I did not force push. I rebased and pull pushed. What should I do to solve this problem?

@amyeroberts
Copy link
Collaborator

amyeroberts commented Jun 12, 2023

@y3sar To rebase onto main you need to force push, as rebasing is a form of history rewrite. The steps are - running on this branch:

  • Get the most recent version of main: git fetch upstream main
  • Rebase: git rebase upstream/main
  • Push changes: git push -f

@y3sar y3sar force-pushed the add-multimodal-docqa-in-task_summary branch from 32d0e00 to 87831bf Compare June 12, 2023 18:58
@y3sar
Copy link
Contributor Author

y3sar commented Jun 12, 2023

@amyeroberts ffmpeg bug still remains. What can I do to solve this?

@amyeroberts
Copy link
Collaborator

@y3sar You'll want to modify this line to:

"sudo apt-get -y update && sudo apt-get install -y libsndfile1-dev espeak-ng time ffmpeg",

@y3sar
Copy link
Contributor Author

y3sar commented Jun 14, 2023

@amyeroberts should I commit in this branch or should I create another pull request?

@amyeroberts
Copy link
Collaborator

@y3sar Commit on this branch :)

@y3sar y3sar force-pushed the add-multimodal-docqa-in-task_summary branch from 87831bf to 10c08ac Compare June 14, 2023 19:36
@huggingface huggingface deleted a comment from github-actions bot Jul 10, 2023
@amyeroberts
Copy link
Collaborator

@y3sar Apologies for the delay with this PR. Could you rebase again and push. We've been having issues with timeouts on the CI which should now be resolved. Additionally, could you update the extension for the .mdx to .md please?

@y3sar
Copy link
Contributor Author

y3sar commented Jul 11, 2023

@amyeroberts thank you for remembering this pull request 😄😄😄🙏
Yes ma'am.

@y3sar y3sar force-pushed the add-multimodal-docqa-in-task_summary branch from 10c08ac to e7433c4 Compare July 11, 2023 13:42
@y3sar
Copy link
Contributor Author

y3sar commented Jul 11, 2023

@amyeroberts looks like the timeout issue remains. Should I change the code example?

@amyeroberts
Copy link
Collaborator

@y3sar Let's draft in help from the king of tests @ydshieh :)

I did a test run of this example on a CPU and it took 40s, so not fats but not super slow either. I think we could try:

  • Another checkpoint. Are there any other smaller DocQA checkpoints we could use?
  • Forcing this example to be skipped in the tests

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 11, 2023

Let me see what's happening on the CI runner.

@ydshieh ydshieh self-assigned this Jul 11, 2023
@ydshieh
Copy link
Collaborator

ydshieh commented Jul 11, 2023

Hi, I checked. That task_summary.md is considered as a single test by pytest , but it has multiple examples (and the checkpoints are not always small).

I can change the environment variable to avoid this situation. Once that change is merged, you can rebase and the CI should be green.

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 11, 2023

The PR is opened

#24753

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 11, 2023

That PR is merged into main. If you pull the latest main and rebase on it, we should be good to merge this PR.

@y3sar y3sar force-pushed the add-multimodal-docqa-in-task_summary branch from e7433c4 to efd6e39 Compare July 11, 2023 23:44
@y3sar
Copy link
Contributor Author

y3sar commented Jul 12, 2023

Thank you the king of tests and @amyeroberts. Should I check for a smaller checkpoint?

@ydshieh
Copy link
Collaborator

ydshieh commented Jul 12, 2023

Should I check for a smaller checkpoint?

It would be always great to use a small(er) checkpoint for testing (if there is any) 🙏 Thank you @y3sar

@y3sar
Copy link
Contributor Author

y3sar commented Jul 14, 2023

@ydshieh @amyeroberts I have found some small(er) models. The model that is being used currently is 803 mb. I have found a checkpoint that is 500 mb. Which is still very big but smaller. And also I have found an even smaller checkpoint that is used for testing by the huggingface internals. But the results are not that reliable.

@amyeroberts
Copy link
Collaborator

@y3sar Thanks for looking into this! The tiny models are just for internal testing and probably not something we want to have in the docs (we want to be able to adapt to our needs without considering breaking changes). I'd say go for the 500MB one :)

@y3sar y3sar force-pushed the add-multimodal-docqa-in-task_summary branch from efd6e39 to 98ac868 Compare July 15, 2023 06:27
@amyeroberts
Copy link
Collaborator

@y3sar Thanks again for iterating and adding this!

@amyeroberts amyeroberts merged commit 4f08887 into huggingface:main Jul 17, 2023
18 checks passed
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
…y.mdx (huggingface#23318)

* add multimodal heading and docqa

* fix sentence

* task_summary data type = modality clarification

* change the multimodal example to a smaller model
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.

None yet

5 participants