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

Add a callback to get_parameter_data to follow data loading #4688

Merged
merged 25 commits into from
Nov 28, 2022

Conversation

edumur
Copy link
Contributor

@edumur edumur commented Oct 4, 2022

Hi all,

In the effort to use qcodes as the data loader of the pyplotter I added a callback to get_parameter. This is usefull to track the progress of the data download.
Since sqlite3 does not allow to keep track of the data loading progress, we compute how many sqlite requests correspond to a certain percentage of progress which is dictated by a config parameter. Then we perform x sql request instead of one, running the callback everytime.

The whole process add an overhead of ~13% on my laptop when used which is not negligible.
However, I would argue that keeping track of the download progress for a GUI is precious and is worth the overhead.

To test the download time

import qcodes as qc
from time import time

def callback(progress):
    # print(progress)
    pass

qc.initialise_or_create_database_at('test.db')

start = time()
qc.load_by_id(2).get_parameter_data('phase')
stop = time()
dt1 = stop-start
print(dt1)
start = time()
qc.load_by_id(2).get_parameter_data('phase', callback=callback)
stop = time()
dt2 = stop-start
print(dt2)

print(dt2/dt1)

I kept the change minimal hoping other people will have comments.

@edumur
Copy link
Contributor Author

edumur commented Oct 12, 2022

Hi all,

I am writing to keep the thread and see if anyone has comments on it.

See you.

@edumur
Copy link
Contributor Author

edumur commented Oct 21, 2022

Hi all,

I am back again.
The thing is that this pull request will allow pyplotter to load the data via qcodes so I am eager to see it go through ^^' .

Hoping to see someone on the thread.

@jenshnielsen jenshnielsen self-requested a review October 24, 2022 13:28
Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I have been very busy with other things. I left some comments inline.

In general I am curious about the types of datasets that you are loading and the slowdown you are seeing in loading them. Ideally I would like it not to be an issue to load a dataset that makes it worthwhile to have a progress bar. Can you say something about the typical number of rows and parameters in your datasets?

qcodes/configuration/config.py Outdated Show resolved Hide resolved
qcodes/dataset/data_set_protocol.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite/query_helpers.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite/query_helpers.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite/queries.py Outdated Show resolved Hide resolved
qcodes/dataset/sqlite/queries.py Outdated Show resolved Hide resolved
@edumur
Copy link
Contributor Author

edumur commented Oct 26, 2022

test.zip
Please find the test database and the test file so that you can see the difference with and without callback.

@jenshnielsen
Copy link
Collaborator

Thanks starting to look ready. Could you resolve the conflicts either by merging or rebasing against master, revert the changes to many and many_many and write a small changelog note. Then it should be ready to go

@edumur
Copy link
Contributor Author

edumur commented Oct 27, 2022

All should be done.
I had trouble incorporating Florian Blanchet changes from commit b476b79, I hope everything is alright.

When I benchmarked the merged code I saw that:

  1. SQL request where 2 to 3 times faster than before the merging (maybe an effect of Florian's work).
  2. The overhead increased from 20% to 50% while using the callback.

image

@jenshnielsen
Copy link
Collaborator

@edumur Looks like some mypy issues have creapt in. will you be able to have a look?

@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #4688 (83c828e) into master (544295d) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4688      +/-   ##
==========================================
+ Coverage   68.27%   68.33%   +0.06%     
==========================================
  Files         339      339              
  Lines       32087    32123      +36     
==========================================
+ Hits        21906    21952      +46     
+ Misses      10181    10171      -10     

@edumur
Copy link
Contributor Author

edumur commented Nov 14, 2022

I had to add callback: Callable[[float], None] | None = None, in get_parameter_data in data_set_in_memory.
But I do not know what I did here so please someone check.

@jenshnielsen
Copy link
Collaborator

The types looks good. Left a few comments inline. There is now a test.db file which should not be committed

iteration is directly taken from config
@edumur
Copy link
Contributor Author

edumur commented Nov 24, 2022

Hi all,

All things seem correct (I think).
I do not understand the "Lint with Darker" error and I do not know what it is checking ^^'.

@jenshnielsen
Copy link
Collaborator

Darker runs the black formatter on only the part of the code that you have changed. We do this because we would like to move to a more consistent formatting but not rewrite all out code in one go losing history. You can run these lints automatically by installing the pre-commit hooks defined in https://github.com/QCoDeS/Qcodes/blob/master/.pre-commit-config.yaml using the pre-commit tool. https://pre-commit.com/ For now don't worry about it and I will run it when merging

* Handle small database with less than 100 rows
* Handle non commensurable tuple of row to be downloaded with percentage of progress
* Adapt test for small database with less than 100 rows
@edumur
Copy link
Contributor Author

edumur commented Nov 28, 2022

I really appreciate you spending time checking this.
I updated the code so that it works now.

I tested it with various database and various percentage and so far it worked 🤞.

@jenshnielsen jenshnielsen enabled auto-merge (squash) November 28, 2022 12:07
@jenshnielsen jenshnielsen enabled auto-merge (squash) November 28, 2022 14:01
@jenshnielsen jenshnielsen merged commit 34715d9 into microsoft:master Nov 28, 2022
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