Skip to content
Snippets Groups Projects

JF correct json format error, and weird fix only if calibration_metadata.yml exists

Merged Karim Ahmed requested to merge fix/small_fixes_while_testing_3.5.0b2 into master
1 unresolved thread

Sorry for mentioning this weird fix again.

But I should have added this only if the file already exists.

There was as well a JSON error done to the notebook after I applied a suggestion through gitlab on JUNGFRAU notebook.

Description

How Has This Been Tested?

This was tested as a part of the beta release test for tags 3.5.0b2 and 3.5.0b3

Relevant Documents (optional)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

Reviewers

@danilevc @schmidtp

Edited by Karim Ahmed

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • I would merge this by the end of the day if there are no comments to address.

    • That makes surprisingly sense. Do we understand why it slipped the earlier tests? Was it just an unlikely scenario?

    • I tested it only while investigating the issue. which was of course when there was an available calibration_metadata.yml

      Then the same situation for proposal 900113 tests (tag 3.5.0b1) on max-exfl017. the corrected folder had a yml file along with the previously corrected files.

      Edited by Karim Ahmed
    • And writing it when processing for the first time created an empty file, breaking the assumption in other places about what keys it always has?

    • Hmm, Ok, this MR doesn't make sense for me any more. I cant even find the request with which I got the error: calibration_metadata.yml doesnt exist.

      That's why I canceled what I wrote before on why it was slipped. as this did not slip in past tests. it is not even an issue.

      Sorry for the confusion. Unfortunately, I haven't copied the report error message were I received this issue. But I can't find it.

      Shall I revert these changes in this MR. or leave it. The only benefit is that we are doing the weird fix only when a file exists (which is the only case when the malformed yaml file occurred.)

      Sorry for the confusion :(

    • Tl;dr I opened this MR after I thought there was an error on how we were using save() even when the calibration_metadata.yml doesn't exists. This is not the case, but I would leave it as it limiting the "fix" for malformed calibration_metadata.yml only when a file exists (which is only tested case for the yml error)

      Edited by Karim Ahmed
    • Uhm... so I understand correctly that with both this version and the one already merged, the corruption issues cannot be reproduced? But there does not seem (yet!) to be a problem with either version?

    • So for the part related to tools.py in this MR, I would say that it offers a refactor more than a fix and it does the same thing as the merged MR.

      The corruption issue is reproducible with the deployed pycalibration in production.

      Edited by Karim Ahmed
    • Well, they are not identical in their logic. This MR will not write a file if there is none to start with. As long as this is fine (I will not comment whether it should, gave up my sanity with this issue long ago), LGTM

    • Please register or sign in to reply
  • Karim Ahmed changed title from JF correct json format error, and wierd fix only if calibration_metadata.yml exists to JF correct json format error, and weird fix only if calibration_metadata.yml exists

    changed title from JF correct json format error, and wierd fix only if calibration_metadata.yml exists to JF correct json format error, and weird fix only if calibration_metadata.yml exists

  • Karim Ahmed changed milestone to %3.5.0

    changed milestone to %3.5.0

  • merged

  • Karim Ahmed mentioned in commit 7440883a

    mentioned in commit 7440883a

Please register or sign in to reply
Loading