daf_persistence Tickets/dm 8686 #46
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andy-slac
approved these changes
Feb 23, 2017
|
|
||
| Returns | ||
| ------- | ||
| ButlerLocaiton or a list of ButlerLocation |
There was a problem hiding this comment.
First ButlerLocaiton is misspelled.
| If no locaiton was found for this map operation, the derived mapper | ||
| class may raise a lsst.daf.persistence.NoResults exception. Butler | ||
| catches this and will look in the next Repository if there is one. | ||
| """ | ||
| func = getattr(self, 'map_' + datasetType) |
There was a problem hiding this comment.
This will throw AttributeError if method is not there, should this be listed in docstring too? Or is it not supposed to happen?
|
|
||
| import yaml | ||
|
|
||
| from . import LogicalLocation, Persistence, Policy, StorageList, Registry, Storage, RepositoryCfg, safeFileIo | ||
| from . import LogicalLocation, Persistence, Policy, StorageList, Registry, Storage, RepositoryCfg, \ | ||
| safeFileIo, ButlerLocation |
There was a problem hiding this comment.
You can do
from . import (LogicalLocation, Persistence, Policy, StorageList, Registry, Storage, RepositoryCfg,
safeFileIo, ButlerLocation)
then you don't need that ugly backslash 😄
| string | ||
| A relative path that describes the path from fromUri to toUri. | ||
| """ | ||
| return os.path.relpath(toUri, fromUri) |
There was a problem hiding this comment.
Is URI a URI or filesystem path? Can this handle file:///path?
| """Implementaion of PosixStorage.exists for ButlerLocation objects.""" | ||
| storageName = location.getStorageName() | ||
| if storageName not in ('BoostStorage', 'FitsStorage', 'PafStorage', | ||
| 'PickleStorage', 'ConfigStorage', 'FitsCatalogStorage'): |
There was a problem hiding this comment.
Is this list of magic names going to grow, maybe move it to more visible location?
There was a problem hiding this comment.
it's not going to grow, at least not much. We are planning to add a pluggable system for storageName functionality.
| @@ -61,16 +87,16 @@ def v1Constructor(loader, node): | |||
| """ | |||
| d = loader.construct_mapping(node) | |||
| cfg = RepositoryCfg(root=d['_root'], mapper=d['_mapper'], mapperArgs=d['_mapperArgs'], | |||
| parents=d['_parents'], policy=d.get('_policy', None)) | |||
| parents=d['_parents'], policy=d.get('_policy', None), deserialzing=True) | |||
| :return: a Storage subclass instance. | ||
| Parameters | ||
| ---------- | ||
| uri - string |
There was a problem hiding this comment.
I think canonical format is uri : string. And generally I think we are supposed to use backticks around types in docstrings.
tests/repository.py
Outdated
| @@ -718,7 +724,7 @@ def test(self): | |||
| mapper=MapperForTestWriting, | |||
| mapperArgs=None, | |||
| parents=[os.path.join(ROOT, | |||
| 'TestOutputAlreadyHasParent/a'),], | |||
| 'TestOutputAlreadyHasParent/a'), ], | |||
There was a problem hiding this comment.
That comma is not really needed there?
| parentRepoData = self._repos.byCfgRoot[parentCfgRoot] | ||
| parents.append(parentRepoData) | ||
| parents.extend(self._getParentRepodDatas(parentRepoData)) | ||
| return parents |
There was a problem hiding this comment.
I'd probably make it a generator.
| if parentRepoData.cfg.mapper == repoData.cfg.mapper: | ||
| if not parentRepoData.repo: | ||
| raise RuntimeError("Parent repo should be initialized before child repos.") | ||
| registry = parentRepoData.repo.getRegistry() |
n8pease
force-pushed
the
tickets/DM-8686
branch
from
7a4e0de
to
e504a75
Compare
n8pease
force-pushed
the
tickets/DM-8686
branch
2 times, most recently
from
f214ea4
to
33bdae6
Compare
The test used to rely on mapper checking before trying to load repos. The Butler has changed so that this no longer works, fix the test to actually create repos.
move CameraMapper.parentSearch impl to PosixStorage
PosixStorage 1. sets up the output repo 2. makes a _parent symlink from output to input I'm not sure what will have to be done for other storages.
adds ability to not follow _parent symlink in parentSearch. adds unit test
Also remove commented code
this allows resources e.g. the registry to be fetched from parent repos, even if the parent is of a write-only repo (that is, even if the parent is not used as an input)
The parent rule is "parents of writable repositories must be inputs". This might end up being a little tedious, as if an existing output is loaded a second time, all the parents of that output must exactly match the inputs passed to Butler.__init__. This might be acceptable in our environment as reloaded Butler output repositories will be loaded by scripts that I think will load all the same repos as needed multiple times. If it's not ok, we can add a stage that preloads the parents of outputs as inputs before the final parents list is needed to be known.
n8pease
force-pushed
the
tickets/DM-8686
branch
from
33bdae6
to
3c6f5a8
Compare
can't add parents to an existing output repository
n8pease
force-pushed
the
tickets/DM-8686
branch
from
a42ea2f
to
5661e43
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.