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

refactor: convert prepare-test-weights.sh to python #152

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

brycedrennan
Copy link
Contributor

@brycedrennan brycedrennan commented Dec 14, 2023

This does feel a lot nicer to run but it is admittedly a mostly useless refactor (in that the previous version was mostly working).

This is in preparation for wanting to re-use components of this for a similar script that outputs all the needed mapping to do load-time conversion.

Ran successfully to completion. But on a repeat run convert_unclip didn't pass the hash check for some reason.

Features

  • fixes inpainting model download urls. some of the urls for the bash script were incorrect and the errors ignored
  • skips downloading inpainting safety model and text encoder since we don't need to convert those
  • shows a progress bar for downloads
  • skips downloading existing files
  • uses a temporary file to prevent partial downloads
  • can do a dry run to check if url is valid DRY_RUN=1 python scripts/prepare_test_weights.py
  • displays the downloaded file hash
  • checks if converted weights exist, skips conversion if hash matches

Further refactoring could be done:

  • simplify the downloads to just a list of files and destinations (and possibly hashes)
  • use commit references instead of main branch names in urls for better reliability
  • put the converted weights in a separate folder

But I wanted to leave the script in a format that would still look somewhat recognizable.

Screenshots
image
image
image

@brycedrennan brycedrennan changed the title refactor: convert bash script to python refactor: convert prepare-test-weights.sh to python Dec 14, 2023
@catwell catwell added the run-ci Run CI label Dec 14, 2023
@catwell catwell added run-ci Run CI and removed run-ci Run CI labels Dec 14, 2023
@catwell catwell added run-ci Run CI and removed run-ci Run CI labels Dec 14, 2023
Copy link
Member

@catwell catwell left a comment

Choose a reason for hiding this comment

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

check if converted weights exist and match hash. skip if thats the case

This one would be useful (in combination with skipping the downloads if files exists, it will make re-running the script very cheap). I suggest you add it.

I tried the script and conversion failed for SAM (I did not yet investigate if this is due to the script or not):

Screenshot from 2023-12-14 18-30-04

You will also need to change CONTRIBUTING.md which references the old script.

scripts/prepare_test_weights.py Show resolved Hide resolved
@catwell
Copy link
Member

catwell commented Dec 14, 2023

I tried the script and conversion failed for SAM (I did not yet investigate if this is due to the script or not):

This is unrelated with your changes, so you can ignore it (it fails on a specific machine, the difference is probably due to something related to torch).

Ran successfully to completion. But on a repeat run `convert_unclip` didn't pass the hash check for some reason.

- fix inpainting model download urls
- shows a progress bar for downloads
- skips downloading existing files
- uses a temporary file to prevent partial downloads
- can do a dry run to check if url is valid `DRY_RUN=1 python scripts/prepare_test_weights.py`
- displays the downloaded file hash
@brycedrennan
Copy link
Contributor Author

brycedrennan commented Dec 15, 2023

check if converted weights exist and match hash. skip if thats the case

Done.

@brycedrennan brycedrennan requested a review from catwell December 15, 2023 03:56
Copy link
Member

@catwell catwell left a comment

Choose a reason for hiding this comment

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

There are still things that could be improved, for instance: why shell out to curl in convert_preprocessors instead of using download_file?

However in its current state it works and it is undeniably an improvement, so let's merge and change it later if needed!

@catwell catwell added run-ci Run CI and removed run-ci Run CI labels Dec 15, 2023
@catwell catwell merged commit 5ca1549 into finegrain-ai:main Dec 15, 2023
1 check passed
@brycedrennan brycedrennan deleted the bd/test-weights branch December 15, 2023 13:36
@brycedrennan
Copy link
Contributor Author

I used chatgpt to do the first pass at the conversion to python and then went and cleaned it up a lot. Missed that curl call- my bad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-ci Run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants