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

MedHelm: Add VQA-RAD scenario and specs #3246

Open
wants to merge 14 commits into
base: med-helm
Choose a base branch
from

Conversation

sashimono-san
Copy link

The VQA-rad scenario is implemented using data from this huggingface dataset. My question regarding this is whether we should instead point to where the data was originally published instead: Open Science Framework Homepage

The PR was tested with HuggingFaceM4/idefics2-8b:

helm-run --run-entries vqa_rad:model=HuggingFaceM4/idefics2-8b --suite vqarad --max-eval-instances 1

Besides the scenario and specs, this PR includes an addition to the documentation, explaining how to get helm running on cloud computes that have network mounted directories, which leads to sqlite3.OperationalError: database is locked.

Copy link
Collaborator

@yifanmai yifanmai left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good in general. However, it looks like your pull request does not pass the linter. Could you install the [dev] extras (or you can install the linter directly using pip install black==24.3.0 flake8==5.0.4 mypy==1.5.1) and then run ./pre-commit.sh, and then make the requested fixes?

Comment on lines +86 to +89
extra_data={
"id": index,
"image_path": image_path,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra_data is unused and should be deleted. If you need to keep ID, you can set the id field (e.g. id=f"id{id}") on the Instance itself.

@sashimono-san
Copy link
Author

Thanks, this looks good in general. However, it looks like your pull request does not pass the linter. Could you install the [dev] extras (or you can install the linter directly using pip install black==24.3.0 flake8==5.0.4 mypy==1.5.1) and then run ./pre-commit.sh, and then make the requested fixes?

No worries, I'll work on that. Thank you for reviewing it!

@yifanmai
Copy link
Collaborator

yifanmai commented Jan 8, 2025

Also, in general it's okay to use unofficial third part mirrors of datasets on Hugging Face; we already do this for some other scenarios.

@sashimono-san sashimono-san changed the base branch from main to med-helm January 14, 2025 08:37
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.

6 participants