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

Fix issues with infered_from and add checks for infered_from and depends_on that are strings #1453

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jan 23, 2019

Fixes #1448

Perform db upgrade to correct:

  • infered_from was passed as a string not a list of strings
  • infered_from was not handled correctly for parameters that were neither deps nor dependents
    In addition:
  • Add logic to catch infered_from and depends_on being passed as a string.
  • move db generation scripts to their own repo to disentangle qcodes git hash from the code using it. Otherwise it is very painful to change test fixtures for older db versions

As noted below I have not found a good way to get mypy to catch this kind of error.
You cannot use List[Union[ParamSpec, Str]] as list is an invariant type python/mypy#3351, the current type checking does not catch is since str in it self is a Sequence

@WilliamHPNielsen could you do a review

@jenshnielsen jenshnielsen changed the title Validate that infered_from adn depends_on are not strings but sequences Validate that infered_from and depends_on are not strings but sequences Jan 23, 2019
@codecov
Copy link

codecov bot commented Jan 23, 2019

Codecov Report

Merging #1453 into master will increase coverage by 0.06%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #1453      +/-   ##
==========================================
+ Coverage   73.79%   73.86%   +0.06%     
==========================================
  Files          92       92              
  Lines       10374    10410      +36     
==========================================
+ Hits         7656     7689      +33     
- Misses       2718     2721       +3

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.

Great!

@astafan8
Copy link
Contributor

  • And a test for ParamSpec?
  • And what about the need for another db upgrade that fixes the outcome of _2to3_get_paramspecs? Or shall we just do it via a fix_* function?
  • Also, what to do with type declarations so that mypy catches this?

@jenshnielsen
Copy link
Collaborator Author

  • Agree on the test.
  • I think we need an upgrade. We can perform the fix now because we still have the redundant information
    in the other table but once we do the upgrade to drop that table this needs to be fixed beforehand. I.e in a previous upgrade. If it's in a fix we may not catch all instances before doing the dropping upgrade.

Mypy:

I expected that

S = TypeVar('S')
Listuple = Union[List[S], Tuple[S, ...]]

But that does not seem to be that simple.

@jenshnielsen jenshnielsen force-pushed the fix/validate_paramspec_input_infered branch from 134de3b to 4217ba0 Compare January 25, 2019 09:07
@jenshnielsen
Copy link
Collaborator Author

You cannot meaningfully use a List of a Union unfortunately python/mypy#3351

@jenshnielsen jenshnielsen force-pushed the fix/validate_paramspec_input_infered branch from d1a9e21 to f68c853 Compare February 4, 2019 09:36
@jenshnielsen jenshnielsen changed the title Validate that infered_from and depends_on are not strings but sequences Fix issues with infered_from and add checks for infered_from and depends_on that are strings Feb 4, 2019
Copy link
Contributor

@Dominik-Vogel Dominik-Vogel left a comment

Choose a reason for hiding this comment

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

Great work!

qcodes/dataset/sqlite_base.py Show resolved Hide resolved
qcodes/dataset/sqlite_base.py Show resolved Hide resolved
@WilliamHPNielsen
Copy link
Contributor

Modulo a few comments above, this looks ready.

@jenshnielsen jenshnielsen merged commit 77617bc into microsoft:master Feb 6, 2019
@jenshnielsen jenshnielsen deleted the fix/validate_paramspec_input_infered branch February 6, 2019 12:59
giulioungaretti pushed a commit that referenced this pull request Feb 6, 2019
Merge: 3b93f62 778de4d
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Merge pull request #1453 from jenshnielsen/fix/validate_paramspec_input_infered
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.

Documentation error v0.1.11: load_or_create_experiment
4 participants