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

[core only] Development/dependencies dsl #1227

Merged

Conversation

WilliamHPNielsen
Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen commented Aug 10, 2018

This is some internal core dev thing.

Introduction

Here is my proposal for a first iteration DSL to describe dependencies.

The idea is simple: let's use what we already have (and then build on that later). What is it that we have? The ParamSpecs. The proposal is therefore to simply write down the ParamSpec in a yaml json file. That is, in all its glory, the DSL.

This PR introduces one two objects and two functions, an InterDependencies object consuming ParamSpecs and RunDescriber object and the functions interdeps_to_yaml and yaml_to_interdeps with methods to_json/from_json. Note that yaml is also supported.

Regarding implementation (to be done in following PRs):

If we want to pursue this, the proposal then includes moving all information from the dependencies and layouts table into a yaml file stored in a column in the runs table (just like snapshot). I'd further propose that we

  • Update the table schema
  • Move all validation logic of a set of ParamSpec (are all setpoints present, are there cyclic dependencies, etc.) into the InterDependencies object. The Measurement class can then use that.
  • Refactor the following methods to no longer query dependencies and layout but read and parse the yaml file instead:
    • get_setpoints
    • get_layout
    • get_dependents
    • get_dependencies
    • get_paramspec
  • Extend the ParamSpec as extensions are needed. One extension comes to mind:
    • ~~Give the ParamSpec a run_id (preferably the global one) ~~

@QCoDeS/core, let us see if this can work for us.

@codecov
Copy link

codecov bot commented Aug 10, 2018

Codecov Report

Merging #1227 into master will increase coverage by 0.62%.
The diff coverage is 92.61%.

@@            Coverage Diff             @@
##           master    #1227      +/-   ##
==========================================
+ Coverage   72.84%   73.47%   +0.62%     
==========================================
  Files          74       76       +2     
  Lines        8588     8866     +278     
==========================================
+ Hits         6256     6514     +258     
- Misses       2332     2352      +20

@WilliamHPNielsen
Copy link
Contributor Author

There is now a little notebook showing the basic idea of object -> yaml and yaml -> object.



def yaml_to_interdeps(yaml_str: str) -> InterDependencies:
yaml = YAML()
Copy link
Contributor

Choose a reason for hiding this comment

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

does this module support the case when ParamSpec class changes but older yaml file is read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does now, via versioning. The code contains an example of a somewhat contrived upgrade, but it establishes the principle.

@astafan8
Copy link
Contributor

nice. it looks neat. are the "bullet points" going to be implemented in this PR?

@WilliamHPNielsen
Copy link
Contributor Author

@astafan8, yes, the idea is to make this PR give us an MVP. Sorry, I think I forgot write that. Let me do it now: we have decided to implement the DSL into the database, meaning that the four bullet points will be covered.

@astafan8
Copy link
Contributor

astafan8 commented Sep 18, 2018

perhaps a useful note: I was looking at ruamel, and on their pypi page they state that

For production systems you should pin the version being used with ruamel.yaml<=0.15

Does it impact us in any way?

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

Sorry that I haven't reviewed the queries and the upgrade algorithm before. The most important comments are about them.

qcodes/dataset/descriptions.py Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Show resolved Hide resolved
qcodes/tests/dataset/test_dataset_basic.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Show resolved Hide resolved
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

I think it's good to go!

@WilliamHPNielsen WilliamHPNielsen merged commit 9c61748 into microsoft:master Oct 26, 2018
giulioungaretti pushed a commit that referenced this pull request Oct 26, 2018
Merge: b11e487 b757bb6
Author: William H.P. Nielsen <whpn@mailbox.org>

    Merge pull request #1227 from WilliamHPNielsen/development/dependencies_DSL
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