Python Wrapper: Implement Import Manager #7084
Conversation
N-o-Z
added
exclude-changelog
|
|
||
|
|
||
| class ImportManager: | ||
| """ | ||
| Manage an import operation on a given repository | ||
| ImportManager provides an easy-to-use interface to perform imports with multiple sources. | ||
| It provides both synchronous and asynchronous functionality TBD |
| # Bug in import API - message is required | ||
| self.commit_message = commit_message if commit_message is not None else "" |
There was a problem hiding this comment.
Is there any intention to make message required in importAPI? if not? what is the value in having optional that is casted to an empty string?
There was a problem hiding this comment.
It's a bug in the API. Commit message should be an optional field we need to fix that. I'll open an issue on it
There was a problem hiding this comment.
On second though - fixed in the wrapper
| if self._in_progress: | ||
| raise ImportManagerException("Import in progress") | ||
|
|
There was a problem hiding this comment.
Believe this is more of a product question: are we allowed to call an import twice (i.e call (start,wait) twice ) currently every time wait is done, the fields are reset and we're ready to do another import with this manager with the same commit_message and metadata.
Also the import_id isn't erased so status will still return the status of the previous run. something feels a bit inconsistent
There was a problem hiding this comment.
The import id isn't erased by design so that you can continue to status on the import process even when it was completed.
I was considering making the mgr a single use object but eventually decided this would just make the user experience more burdensome. My assumption is that if we wanted it to be single use - it's better to just replace it with a function call rather then an object
There was a problem hiding this comment.
We can always modify this behavior in the future as this is not a guarantee we have to commit to
There was a problem hiding this comment.
We can always modify this behavior in the future as this is not a guarantee we have to commit to
| if not self._in_progress: # No import in progress - nothing to do | ||
| return | ||
|
|
||
| with api_exception_handler(): | ||
| self._client.sdk_client.import_api.import_cancel(repository=self._repo_id, | ||
| branch=self._branch_id, | ||
| id=self._import_id) | ||
| self._in_progress = False | ||
| self.sources = [] |
There was a problem hiding this comment.
Also here...
If I run import wait enough time (for the import to finish) and then call cancel, I will get a conflict saying import is done, but if I call wait and then call cancel everything is fine and no error returns
There was a problem hiding this comment.
Modified the behavior for cancel to not check for in progress
| mgr = branch.import_data("my imported data", metadata={"foo": "bar"}) | ||
|
|
||
| # No import running | ||
| mgr.cancel() |
There was a problem hiding this comment.
When should cancel succeed and when should it return an error?
There was a problem hiding this comment.
Basically always succeed unless the import was canceled unexpectedly by someone else or the import started and already completed
| assert res.commit.message == branch.commit_message() | ||
| assert res.commit.metadata == branch.metadata() |
There was a problem hiding this comment.
We might want to add a validation that it used the commit_message and metadata we expected
"my imported data", metadata={"foo": "bar"}
| mgr.start() | ||
| sleep(1) | ||
| mgr.cancel() |
There was a problem hiding this comment.
This might be flaky if we somehow finished before the cancel call
There was a problem hiding this comment.
The import here is very big, it will not take less than a second. We have a similar test flow in esti
…-import-manager-7077 # Conflicts: # clients/python-wrapper/lakefs/branch.py # clients/python-wrapper/lakefs/namedtuple.py # clients/python-wrapper/lakefs/repository.py
Closes #7077
Change Description
Implement ImportManager module
Testing Details
Unit and integration tests