Refactoring/#146 use new model save functionality #186
Conversation
…ave_functionality' into refactoring/#146_use_new_model_save_functionality # Conflicts: # tests/fixtures/model_fixture.py # tests/unit_tests/udfs/test_base_udf.py
exasol_transformers_extension/udfs/models/model_downloader_udf.py
Outdated
Show resolved
Hide resolved
exasol_transformers_extension/udfs/models/model_downloader_udf.py
Outdated
Show resolved
Hide resolved
| @@ -61,7 +60,7 @@ def upload_to_bucketfs(self) -> Path: | |||
|
|
|||
| returns: Path of the uploaded model in the BucketFS | |||
| """ | |||
| return self._bucketfs_model_uploader.upload_directory(self._tmpdir_name / "pretrained" / self._model_name) | |||
| return self._bucketfs_model_uploader.upload_directory(self._tmpdir_name / "pretrained" / self._model_name) #todo should we do replace(-,_) here to? | |||
There was a problem hiding this comment.
where did we replace it befoe?
There was a problem hiding this comment.
here:
and then consequently here:
looks like there was concern about the path in bucketfs containing "-". but i dont know if that is still valid.
tests/fixtures/model_fixture.py
Outdated
| upload_model(bucketfs_location, model_name, download_tmpdir) | ||
| yield upload_tmpdir_name | ||
| upload_model(bucketfs_location, model_name, upload_tmpdir_name) | ||
| yield download_tmpdir |
There was a problem hiding this comment.
changed upload_tempdir_name to upload_tempdir. if this is not what you meant please tell me.
There was a problem hiding this comment.
no, I mean that we yield download_tmpdir is confusing me, I would yield the upload_tmpdir, but I might missunderstand something
There was a problem hiding this comment.
i know. especially since we dont change download_tmpdir, and then as far as i can see dont use it again after yielding. but when i tried removing it it broke. i cannot explain why
There was a problem hiding this comment.
ok, I will have a closer look
Co-authored-by: Torsten Kilias <[email protected]>
Co-authored-by: Torsten Kilias <[email protected]>
tests/fixtures/model_fixture.py
Outdated
| bucketfs_location = LocalFSMockBucketFSLocation( | ||
| PurePosixPath(upload_tmpdir)) | ||
| upload_model(bucketfs_location, model_name, upload_tmpdir) | ||
| download_model(model_name, download_tmpdir) |
There was a problem hiding this comment.
ok, but the either change the function name or add a comment. because this does not even pretend to have anything to do with a "local bucketfs", its just saving the model locally.
MarleneKress79789
changed the base branch from
main
to
dev_storage_format_change
…w_model_save_functionality
MarleneKress79789
deleted the
refactoring/#146_use_new_model_save_functionality
branch
* switched use to huggingface transfer save pretrained version
* changed to load local model
* fix local bucketfs model upload
* removed download sample model fixture because of duplication
* documentation
* simplify upload_model_to_local_bucketfs in model_fixture.py
* simplify implementation and improve naming in model_fixture.py
* Remove .replace("-", "_") from model names
Co-authored-by: Torsten Kilias <[email protected]>
* Refactoring/#146 use new model save functionality (#186) * switched use to huggingface transfer save pretrained version * changed to load local model * removed download sample model fixture because of duplication * #147: Removed huggingface token from model loading (#203) * removed token_conn everywhere except model download and tests * removed token_conn from relevant tests * Documentation improvements (#199) * Prepared release 1.0.0 (#206) Co-authored-by: Torsten Kilias <[email protected]>
All Submissions:
[CodeBuild]to the commit messageFixes Use new functions with save_pretrained for model loading #146