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

Make mapper aliases configurable #175

Merged
merged 5 commits into from Feb 13, 2020
Merged

Make mapper aliases configurable #175

merged 5 commits into from Feb 13, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Feb 10, 2020

This PR removes the default StructureMapper aliases and moves them to the default config.ini. They were previously quite annoying as you had to use these pseudo-MP field names if you didn't want to patch the our library yourself (i.e. if you were both creating and consuming your database using the optimade models).

  • Added <endpoint>.aliases sections to config file, with blank defaults, similar to provider fields.
  • ResourceMapper now pulls aliases from three places: the defined ALIASES class data, any config section config.aliases[ENDPOINT] and the config provider fields.

We should probably write some docs on how to use the aliasing features.

@codecov
Copy link

codecov bot commented Feb 10, 2020

Codecov Report

Merging #175 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   86.24%   86.31%   +0.07%     
==========================================
  Files          40       40              
  Lines        1868     1878      +10     
==========================================
+ Hits         1611     1621      +10     
  Misses        257      257
Flag Coverage Δ
#unittests 86.31% <100%> (+0.07%) ⬆️
Impacted Files Coverage Δ
optimade/server/mappers/entries.py 97.77% <ø> (ø) ⬆️
optimade/server/mappers/structures.py 100% <ø> (ø) ⬆️
optimade/server/config.py 94.87% <100%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee15c4a...d1f0ab7. Read the comment docs.

@CasperWA
Copy link
Member

CasperWA commented Feb 10, 2020

I must admit, I thought the point was that one implemented their own StructureMapper class and passing it to where it may be needed - but maybe this is not what you had in mind?

Note, I am not against this at all, I am only confused as to what the desired design goals are at the moment of this package/repository. What its prioritized purpose is (e.g., first and foremost a source for the models and filter grammar, then to be used to implement an index meta-database, then for a full implementation, etc.)

@ml-evs
Copy link
Member Author

ml-evs commented Feb 10, 2020

I must admit, I thought the point was that one implemented their own StructureMapper class and passing it to where it may be needed - but maybe this is not what you had in mind?

Absolutely, but the point should still be to make everything as reusable as possible. I know it's trivial to implement a structure mapper without aliases, but it's even more trivial to, er, not have to in the default case.

Note, I am not against this at all, I am only confused as to what the desired design goals are at the moment of this package/repository. What its prioritized purpose is (e.g., first and foremost a source for the models and filter grammar, then to be used to implement an index meta-database, then for a full implementation, etc.)

I agree with this ranking of the goals but I don't see this minor goal impeding on the rest. I would argue the aliases here are a special case where your fields perfectly align with the OPTiMaDe fields, asdie from their field names. I think a much more likely use case would be someone spinning up a new database for sharing results using the OPTiMaDe models from the get-go.

@CasperWA
Copy link
Member

I must admit, I thought the point was that one implemented their own StructureMapper class and passing it to where it may be needed - but maybe this is not what you had in mind?

Absolutely, but the point should still be to make everything as reusable as possible. I know it's trivial to implement a structure mapper without aliases, but it's even more trivial to, er, not have to in the default case.

For sure. And agreed.

Note, I am not against this at all, I am only confused as to what the desired design goals are at the moment of this package/repository. What its prioritized purpose is (e.g., first and foremost a source for the models and filter grammar, then to be used to implement an index meta-database, then for a full implementation, etc.)

I agree with this ranking of the goals but I don't see this minor goal impeding on the rest. I would argue the aliases here are a special case where your fields perfectly align with the OPTiMaDe fields, asdie from their field names. I think a much more likely use case would be someone spinning up a new database for sharing results using the OPTiMaDe models from the get-go.

Perhaps. And I definitely don't see it impeding either.
In the end I was maybe just starting to re-raise the question of this repository's purpose. But will start an issue on this instead.

@CasperWA CasperWA self-requested a review February 10, 2020 16:36
Copy link
Member

@CasperWA CasperWA 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.
I've suggested some minor code optimizations in config.py.

optimade/server/config.py Outdated Show resolved Hide resolved
optimade/server/config.py Outdated Show resolved Hide resolved
optimade/server/config.py Outdated Show resolved Hide resolved
@ml-evs ml-evs requested a review from CasperWA February 10, 2020 17:43
@ml-evs ml-evs force-pushed the ml-evs/remove_mappers branch 3 times, most recently from 31ecf76 to def3dbb Compare February 10, 2020 17:53
Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @ml-evs

@CasperWA CasperWA merged commit 9f16ef0 into master Feb 13, 2020
@CasperWA CasperWA deleted the ml-evs/remove_mappers branch February 13, 2020 14:04
This was referenced Feb 13, 2020
CasperWA added a commit that referenced this pull request Feb 13, 2020
Bump to v0.5.0

Changes:
- Possibility for Docker deployment for both the index meta-database
  server as well as the regular server (#140, @ltalirz, @CasperWA)
- Test building and starting Docker images with GitHub Actions CI
  (#140, @CasperWA, @ml-evs, @ltalirz)
- Remove `/index` from the index meta-database's base URL
  (#140, @ltalirz, @CasperWA)
- `include` query parameter (#163, @CasperWA)
- Rename `optimade/server/deps.py` to `optimade/server/query_params.py`
  (#163, @CasperWA)
- Human-readable landing page for versioned base URLs, as well as for
  `/optimade` (#172, @ml-evs)
- Move mapper aliases to config file and out of mapper classes
  (#175, @ml-evs)

Bug fixes:
- Properly build versioned base URLs (#178, @CasperWA)
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

2 participants