NREL / OpenStudio

OpenStudio is a cross-platform collection of software tools to support whole building energy modeling using EnergyPlus and advanced daylight analysis using Radiance.
https://www.openstudio.net/
Other
494 stars 188 forks source link

All My Measure XML corrupted when "My Measures" has measure with same UID as downloaded BCL measure #2407

Closed DavidGoldwasser closed 7 years ago

DavidGoldwasser commented 7 years ago

The typical case this would happen is when a measure developer wants to test or use a more current version of their measure than what they have uploaded to BCL. I typically do this for every measure I upload. In 1.x I could have the same UID in BCL directory and in "My Measure Directory: without any issues. They both showed in GUI with correct badge and either or both could be used.

I flagged this as low frequency because it only will affect developers, I marked it as Major despite that because if I goof up UID's of measures in repo for BCL very bad things will happen.

Here are my steps to reproduce this on a mac with a clean machine.

  1. Checkout https://github.com/NREL/OpenStudio-measures
  2. Optionally throw away BCL directory if you already have one. OpenStudio will remake and download OpenStudio Results from BCL after launch. https://bcl.nrel.gov/node/82918
  3. Launch 2x OpenStudio (I'm using 12/1 installer on mac)
  4. On Measures tab click "Sync Project Measures with Library", then click "Update"
  5. go to "Change My Measures Directory" to "NREL working measures" directory within the OpenStudio-measures repo
  6. On Measures tab click "Sync Project Measures with Library", then click "Update"
  7. Check Git Repo to see if XML's are corrupted (see screenshot below)
  8. Switch "My Measure Directory" back to the original one or any other directory.

Repeat steps 4-8 a few times and you should see all more most XML's changed as shown below.

screen shot 2016-12-01 at 12 37 59 pm

If this doesn't happen after a few times, try making a minor change to the measure.rb of OpenStudio Results and then repeat one more time.

If I had other measures in my BCL directory that are also in a repository that I'm using for the "My Measure Directory" I expect I would see the same thing. For most use cases if measure developers know this they can throw away their BCL directory before doing any testing, but not as easy for OpenStudio Results, I would have to work out hack to test that in GUI, which I can do if necessary.

Solutions:

The ideal solution would be the 1x behavior that lets you have a local and BCL version of the same measure (I define same as having same UID) so long as one is in the BCL directory and the other is in the "My Measures Directory".

If we can't do that we should at least only affect the one XML file in my repo, not all of the others. I assume that is a pretty easy fix. I think a step better than that would be to just leave the XML alone in "My Measure Directory" and maybe exclude it from the GUI. Their should at least be a warning that comes up "I just changed the UID on OpenStudio Results in your My Measures Directory, a duplicate UID exists on a downloaded BCL measure".

DavidGoldwasser commented 7 years ago

@asparke2 be aware of this if you commit QAQC measures to repo.

macumber commented 7 years ago

This may be related to https://www.pivotaltracker.com/story/show/133572343

macumber commented 7 years ago

I tried setting a break point in BCLXML::changeUID but was not able to reproduce this on windows

https://github.com/NREL/OpenStudio/blob/os_2_0_develop/openstudiocore/src/utilities/bcl/BCLXML.cpp#L836

macumber commented 7 years ago

I think there is a time issue here, I believe if you start the app (with the openstudio-measures dir set) then immediately switch to the scripts tab and hit update you can trigger this. I believe that moving more slowly may avoid this issue, hoping that @DavidGoldwasser can verify

DavidGoldwasser commented 7 years ago

No luck getting XML's corrupted on quick launch and sync. Tried 5 times so far. Now I'm going to try workflow I describe originally but I'll wait 2 minutes between each step (the expectation being that maybe this will prevent corruption vs. fast version of same workflow). @macumber does that seem like enough time?

DavidGoldwasser commented 7 years ago

@macumber so while I can't force it to break XML's the way you suggested, I have not been able to make the XML's go bad when I follow my workflow but wait 2 minutes between each step. I've spent about 35 minutes switching back and forth, and even changed measure.rb as an extra step.

When I typically run workflow, I'm not trying to break it, I probably wait 3-4 seconds after dismissing a dialog before doing the next step. Typical use for me to switch directories a lot is when I updated measures in a number of directories within a repo, and use the app to update the XML's before committing. Maybe I can just write a script to use measure manager to update XML's outside of GUI.

If we do table this for now, since seems to just affect me, people committing measures to repos just need to take time to look at the change log and in the XML's UID values before committing so they don't corrupt the repo files.

If we do decide to fix this, would you just have a way for measure manager to lock up the GUI until it is done when syncing, or maybe just have a check in "change my measures directory' that can check if measure manager is busy at the moment (that will affect less people than full GUI lockout)?

macumber commented 7 years ago

Yes I think your last idea is what I am leaning towards, you are not going "too fast" in this case, measure manager is too slow with large numbers of measures. I can work to speed that up as well.

DavidGoldwasser commented 7 years ago

Cool. @macumber by the way on a whim I did find another way to trigger this but it is an abnormal use case, but maybe useful if it gives you a way to mimic it on Windows. I clicked the "Sync Project Measures with Library" button 3-4 times without waiting for the dialog to come up. The result is that a few copies of that dialog open, and after clicking "Update" on them all the XML's were messed up. This didn't require me switching directories.

macumber commented 7 years ago

Perfect!

DavidGoldwasser commented 7 years ago

Fixed by commit not listed here.