Repository navigation
Update backend [part 4] - #29
Conversation
|
@r0mainK There are merge conflicts |
|
@vmarkovtsev solved them, ready for review when you get the time |
| client = self.create_client() | ||
| return client.lookup_bucket(self.bucket_name) | ||
|
|
||
| def initialize_backend(self, force): |
There was a problem hiding this comment.
The behavior matches "reset" so better to rename
| if blob.exists() and not force: | ||
| self._log.error("Model %s already exists, aborted.", meta["uuid"]) | ||
| return 1 | ||
| raise ExistingModelError |
There was a problem hiding this comment.
Need to rename to ModelAlreadyExistsError
| if isinstance(where, str): | ||
| os.makedirs(os.path.dirname(where), exist_ok=True) | ||
| f = open(where, "wb") | ||
| def fetch_model(self, source: str, file: str, chunk_size: int =DEFAULT_CHUNK_SIZE) -> None: |
There was a problem hiding this comment.
The type hint for file is wrong, it is a union
There was a problem hiding this comment.
also added typing to the rest of the file
| f = open(where, "wb") | ||
| def fetch_model(self, source: str, file: str, chunk_size: int =DEFAULT_CHUNK_SIZE) -> None: | ||
| self._log.info("Fetching %s...", source) | ||
| r = requests.get(source, stream=True) |
There was a problem hiding this comment.
We should check the http return code here
| except Exception as e: | ||
| log.critical("Failed to load the model: %s: %s" % (type(e).__name__, e)) | ||
| return 1 | ||
| model = GenericModel(source=path, dummy=True) |
There was a problem hiding this comment.
Why did you remove the error handlers? It is a top level function
There was a problem hiding this comment.
here was my mistake, reverting
| model_url = backend.upload_model(path, base_meta, args.force) | ||
| except ModelExistsError: | ||
| return 1 | ||
| model_url = backend.upload_model(path, base_meta, args.force) |
There was a problem hiding this comment.
here I don't understand why, since the error is a custom one that clearly explains whats happening - and logs in upload_model also provide information. Unless you want to sytematically wrap errors this way to return 1 ? In which case we should wrap the other top level functions too, no ?
There was a problem hiding this comment.
Yep we should wrap everything, that was a dirty cutting off the edges
|
Added catches about everywhere, and moved the message to the logger in most cases. I put two TODO for when dulwich implements a porcelain error. Gonna add some test now, to get coverage a bit higher. But yeah ready for more review or merge after that. |
| return 1 | ||
|
|
||
| log.info("Deleting content of index ...") | ||
| backend.index.initialize_index() |
There was a problem hiding this comment.
Future commit: rename initialize_index to reset
| log.info("Deleting content of index ...") | ||
| backend.index.initialize_index() | ||
| try: | ||
| backend.index.upload_index("initilialize", {}) |
| except ExistingBackendError: | ||
| return 1 | ||
|
|
||
| log.info("Deleting content of index ...") |
There was a problem hiding this comment.
Future commit: "Resetting the index"
| template_readme = backend.index.load_template(args.template_readme) | ||
| backend.index.update_readme(template_readme) | ||
| except ValueError: | ||
| return 1 |
There was a problem hiding this comment.
We are catching much code here, so better to split and log what has failed exactly.
The same is true for the rest of the return 1-s
| backend.index.update_readme(template_readme) | ||
| backend.index.upload_index("delete", meta) | ||
| try: | ||
| backend.index.upload_index("delete", meta) |
There was a problem hiding this comment.
Future commit: rename to upload
2 participants
Take out unused methods in backend relating to the index, add
initialize_backendmethod, create helper function in gcs_backend to avoid redundancy