-
Notifications
You must be signed in to change notification settings - Fork 15
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
Versioning_Engine: Add methodology for directly calling upgrades from main process rather than via pipes #3332
Versioning_Engine: Add methodology for directly calling upgrades from main process rather than via pipes #3332
Conversation
@BHoMBot check versioning |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check compliance |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check compliance |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@IsakNaslundBh to confirm, the following actions are now queued:
There are 2 requests in the queue ahead of you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to approve the functionality following the test run under direction of @IsakNaslundBh 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to approve based on
- Code inspection
- Thorough testing by Isak
- Local check from @pawelbaran
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 24 requests in the queue ahead of you. |
2c9ab96
to
01695af
Compare
@IsakNaslundBh to confirm, the following actions are now queued:
There are 46 requests in the queue ahead of you. |
…rsion Potential scope to improve the behaviour compared to previous version, but for this first system change prefer to make the systems behave literally identical to ensure this switch gives as small impact in terms of functionality as possible.
The check |
The check |
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 3 requests in the queue ahead of you. |
FAO: @FraserGreenroyd The check they wish to have dispensation on is unit-tests. If you are providing dispensation on this occasion, please reply with:
|
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 24148984891 |
@IsakNaslundBh I have now provided a passing check on reference |
@BHoMBot check ready-to-merge |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 2 requests in the queue ahead of you. |
NOTE: Depends on
BHoM/Versioning_Toolkit#257
Issues addressed by this PR
Closes #3331
Changes the way the BHoMUpgraders are called from running via pipes to instead dynamically loading up the upgraders and directly calling them.
Initial testing shows that this can give a more than 10x speed increase for de-serialising objects requiring versioning, which can be critical for slightly larger serialised files. Increasingly important after changes like the one made in BHoM/BHoM#1579 as that would significantly slow down the de-serialisation of and json file containing Bar objects. This could mean a difference for a file containing a few 100 bars of 10-30 seconds rather than say 3-5 minutes. Hence think this change is fairly critical.
For this PR I opted to make the events raised and calls make mimic the current system as 1:1 as possible. Scope to clean up some of the event raising and similar, but would prefer to leave that to a second PR as this change is big enough.
Also opted to keep the code for the Pipe system, and having a hard-coded boolean as to which system to be used, to make it very simple to switch back to the old system if a problem is discovered without completely reverting this PR. Long term if this works, I think we can get rid of it, and discussed with @adecler , scope to also significantly simplifying the versioning system, leading to a single dll rather than the 1 exe per version we currently work with, but again, for a later PR.
Test files
Tested through as complete as I could think of here
Test by de-serialising all files in https://github.com/BHoM/Versioning_Toolkit/tree/develop/.ci/code/Versioning_Test/Datasets and then serialise them out again, first using the current system on develop (folder called Old in the test folder) and then doing the same again with the new system on this branch. Whilst doing the runs, all events for a particular object is also recorded and stored out.
Every re-serialised json is then compared, ensuring the content contained is the same. Same is done for the events, ensuring the events raised are identical.
For Methods and adapters, no change found.
For obejcts, the only difference found has been properties with BHoM_Guid for some object types. From investigation it seem to be Guids of things like CustomObjects and a like, where a new Guid has been instantiated during the De-serialisation, hence natural that a difference occurs (would be the same as comparing two runs of the old system with each other).
The only other difference found has to do with Items containing "$date" and relating to objects containing Events from 3.3 and before. For this it is related to this commit: BHoM/BHoM@d5998b2 where the date time was added. And object serialised before that will naturally deserialise to include a new
DateTime = DateTime.UtcNow
which then simply depends on when it was deserialised.On top of this, ofc Versioning check should be passing. Have also added an example json file and script that can be used to check the speed difference.
Changelog
Additional comments