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

DM-24290: PostgreSQL database handles case when namespace is not provided incorrectly. #247

Merged
merged 10 commits into from May 5, 2020

Conversation

DinoBektesevic
Copy link
Collaborator

Solves issue described in DM-24290 point 1.a when namespace is not provided it is set to an non-existent DSN "schema" key. Went unnoticed because all the tests provide explicit namespace arguments.

@DinoBektesevic DinoBektesevic self-assigned this Apr 1, 2020
@DinoBektesevic DinoBektesevic changed the title DM-24290: PostgreSQL database handle case when namespace is not provided incorrectly. DM-24290: PostgreSQL database handles case when namespace is not provided incorrectly. Apr 1, 2020
python/lsst/daf/butler/_butler.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/_butler.py Outdated Show resolved Hide resolved
Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

(Trivial) Registry changes look good to me, and I've only quickly scanned the rest - I'm happy to leave it to you and @timj to work out the URI handling, since I historically haven't followed that closely and would need to spend some time getting up to speed on it to understand the subtleties.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Some minor comments but this does look better

python/lsst/daf/butler/core/location.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/core/location.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/core/location.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/core/location.py Outdated Show resolved Hide resolved
tests/config/basic/s3Datastore.yaml Outdated Show resolved Hide resolved
tests/test_butler.py Outdated Show resolved Hide resolved
tests/test_location.py Outdated Show resolved Hide resolved
@DinoBektesevic DinoBektesevic force-pushed the tickets/DM-24290 branch 2 times, most recently from 39f2c90 to ec0451f Compare April 29, 2020 01:28
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

looks good.

python/lsst/daf/butler/core/config.py Show resolved Hide resolved
python/lsst/daf/butler/core/location.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/core/location.py Outdated Show resolved Hide resolved
@DinoBektesevic DinoBektesevic force-pushed the tickets/DM-24290 branch 2 times, most recently from 55472d0 to bdae5d9 Compare April 29, 2020 18:18
@DinoBektesevic
Copy link
Collaborator Author

After talking with Tim it was decided a clearer way than the ButlerURI.updateFile("") was wanted so ButlerURI.split, ButlerURI.basenameandButlerURI.dirname` was added.

Extra handling of weird edge-cases added.
Fixes for certain situations where URI can, by implication, only be a directory were made.

This finally allows for <butlerRoot> tags in configs to be properly replaced by config file location. URIs without trailing / can finally be used to instantiate a butler too (f.e. Butler("s3://bucket/root"))

@DinoBektesevic
Copy link
Collaborator Author

You also don't want to be trying to reconstruct the URI string like that because you will miss other bits like fragments.

Maybe I'm just slightly confused as to how are fragments meant to be used if we're missing the basename of the URI, but here are the desired changes. I thought magically expanding head value might be confusing when compared to os.path.split behaviour but I can see why making sure it's easy to round-trip through ButlerURI is important too.

@DinoBektesevic DinoBektesevic force-pushed the tickets/DM-24290 branch 2 times, most recently from 0fdb749 to 078d938 Compare May 4, 2020 21:45
When creating Keys in AWS S3 only keys ending with '/' will
appear and behave as directories. Thus, there is a difference
between inserting `/abs/dir` and `/abs/dir/` keys into a Bucket.
This affects creation of Datastore roots and Butler.makeRepo
functionality which will then create non-intuitive paths in S3.

Added dirLike attribute to ButlerURI. Attribute tracks whether
the ButlerURI ends with a separator or not.
Added forceDirectory argument to ButlerURI. When set to True it
will force a trailing separator to exist.
Forced LocationFactory, Butler.makeRepo and Location datastoreRootUri
argument to always forceDirectory. These are places where we *know*
that the given string *must* be a directory.

Rearranged the Location testButlerUri cases because they were hard
to keep track of.

Added explicit additional check on whether a Bucket exists in
Butler.makeRepo. Now raising a ValueError that the bucket doesn't
exist.

Moved setting of Config.configFile to __initFromFile from
__initFromYamlFile.
Removed empty newline and unwanted comments.

Updated all templates in tests/config from '{run}' to '{run:/}'.

Add missing empty line in test_location.
Correctly propagating butlerRoot for S3Datastore required inovative
use of butler.updateFile in less-than-intuitive ways. ButlerURI split
works the same way as os.path.split does and returns the base and dir
names.

This simplifies and makes logic checking whether an URI points to a
config file more expressive.

Added ButlerURI.split method, equivalent to os.path.split.
Added ButlerURI.dirname method, equivalent to os.path.dirname.
Added ButlerURI.basename method, equivalent to os.path.basename.
Added tests for ButlerURI.split.
Handling of edgecases such as empty paths and "." improved.
@DinoBektesevic DinoBektesevic merged commit 835d2b5 into master May 5, 2020
@timj timj deleted the tickets/DM-24290 branch May 17, 2020 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants