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

Unique name for qcodes log file for every process id #1816

Merged
merged 12 commits into from
Nov 15, 2019

Conversation

lakhotiaharshit
Copy link
Contributor

@lakhotiaharshit lakhotiaharshit commented Nov 8, 2019

One possible solution to the logging_error happening because rollover to a new log file being blocked by another process is to uniquely name the logging file for each process id. In this PR apart from adding the process id, I have also added a date to the file to ensure a) file names are as unique as possible b) they are properly arranged and easier to find.

Though this might clutter ~/.qcodes folder with many log files per day.

@astafan8 @jenshnielsen @Dominik-Vogel

To do:

@jenshnielsen
Copy link
Collaborator

Cool, On thing to consider is how we handle the roll over. AFAIK the rollover will append dates to the filename too. Do we end up with two dates in the filename (do we care) is there a better way? Should we just remove the rollover now?

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 ok to keep the roll-over in case a notebook/kernel is long-running, but then the name of the file should not need to include the date, perhaps some magic hash of the date would be ok ))

qcodes/logger/logger.py Outdated Show resolved Hide resolved
qcodes/logger/logger.py Outdated Show resolved Hide resolved
lakhotiaharshit and others added 2 commits November 11, 2019 09:55
Co-Authored-By: Mikhail Astafev <astafan8@gmail.com>
Co-Authored-By: Mikhail Astafev <astafan8@gmail.com>
@lakhotiaharshit
Copy link
Contributor Author

lakhotiaharshit commented Nov 11, 2019

I think it's ok to keep the roll-over in case a notebook/kernel is long-running, but then the name of the file should not need to include the date, perhaps some magic hash of the date would be ok ))

@astafan8 I also think roll-over may be needed for long-running notebook. I can add date dependent uuid. But it will make the names of the log file very messy. Something like this:

6337b0ed417631c2579927a115985bcb-17652-qcodes.log

I have not seen users opening the log files very often so we can live with the messy names. However, the only way to find the relevant log files will by sorting them according to dates.

@astafan8
Copy link
Contributor

However, the only way to find the relevant log files will by sorting them according to dates.

@lakhotiaharshit and by also looking at the output of start_all_logging function, right? at the "usually top" of the notebook

@lakhotiaharshit
Copy link
Contributor Author

However, the only way to find the relevant log files will by sorting them according to dates.

@lakhotiaharshit and by also looking at the output of start_all_logging function, right? at the "usually top" of the notebook

No, It only gives the location of the command_history.log. It will be good to also add the filename of qcodes.log

@astafan8
Copy link
Contributor

No, It only gives the location of the command_history.log. It will be good to also add the filename of qcodes.log

:) is this smth for this PR as well?

@lakhotiaharshit
Copy link
Contributor Author

No, It only gives the location of the command_history.log. It will be good to also add the filename of qcodes.log

:) is this smth for this PR as well?

Yes, this is taken care of in this PR itself. Now the output of start_all_logging() also contains the location of the qcodes log file as shown below:

start_all_logging()
Logging hadn't been started.
Activating auto-logging. Current session state plus future input saved.
Filename : C:\Users\a-halakh.qcodes\logs\command_history.log
Mode : append
Output logging : True
Raw input log : False
Timestamping : True
State : active
Qcodes Logfile : C:\Users\a-halakh.qcodes\logs\6337b0ed417631c2579927a115985bcb-11712-qcodes.log

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.

yes!

@codecov
Copy link

codecov bot commented Nov 11, 2019

Codecov Report

Merging #1816 into master will increase coverage by 0.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #1816      +/-   ##
==========================================
+ Coverage   70.24%   70.25%   +0.01%     
==========================================
  Files         148      148              
  Lines       18591    18598       +7     
==========================================
+ Hits        13059    13066       +7     
  Misses       5532     5532

@Dominik-Vogel
Copy link
Contributor

Dominik-Vogel commented Nov 12, 2019

My impression is that the resulting names are not very practical to use. We will have some long uuids that don't convey any information to the human eye and some dates at the end. So we end up with a directory with a lot of files in random order.
Therefore I would like to suggest an alternative:
[start date][filename from which start_all_logging has been called][PID if needed][additional date from rollover]

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.

This looks good to me.
If you wanted to be extra cautious, you could add a test that initiates multiple python processes that log at the same time. But personally I feel that is not worth the time. So I am all for it the way it is now!

@jenshnielsen jenshnielsen merged commit 708a5f6 into microsoft:master Nov 15, 2019
@lakhotiaharshit lakhotiaharshit deleted the start_all_logging branch January 22, 2020 08:36
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

4 participants