Fix559 Change reference/access of individual pack in the MultiPack from (array) index based to pack_id based (to facilitate delete, insert), with version-enabled backward compatibility #576
Conversation
…ked added (new version as [0,0,1])
…ked added (new version as [0,0,1])
…son logic of yesterday.
|
please provide a description title to the PR |
…dex to pack_id change for multiPack, with new unit test case
…rialization dictionary using string as key issue.
Codecov Report
@@ Coverage Diff @@
## master #576 +/- ##
==========================================
+ Coverage 80.90% 80.95% +0.05%
==========================================
Files 240 240
Lines 17362 17430 +68
==========================================
+ Hits 14047 14111 +64
- Misses 3315 3319 +4
Continue to review full report at Codecov.
|
J007X
changed the title
|
Thanks Hector,
I've just changed to "Fix559 Change reference/access of individual pack in the MultiPack from (array) index based to pack_id based (to facilitate delete, insert), with version-enabled backward compatibility"
Also about today's check in: I've made some changes to 5 places however this might also need a MultiPack change related to deserialization (500/547 related) by Bater tomorrow, so the current commit version is not the fully sync'd / final version.
Thanks,
James
James Xiao
Architect
T: +971 2 8113163
E: ***@***.***
www.mbzuai.ac.ae<http://www.mbzuai.ac.ae>
Mohamed bin Zayed University of Artificial Intelligence
Masdar City, Building 1B, 3rd Floor
Abu Dhabi, UAE
[cid:MBZUAI_Logo_4d283c4f-1a0d-4aed-8a25-b17ff8b431f8.png]
The world’s first specialized research-based AI university
…________________________________
发件人: Hector ***@***.***>
发送时间: 2022年1月11日 3:56
收件人: asyml/forte ***@***.***>
抄送: James Xiao ***@***.***>; Author ***@***.***>
主题: Re: [asyml/forte] Fix559 (PR #576)
please provide a description title to the PR
―
Reply to this email directly, view it on GitHub<#576 (comment)>, or unsubscribe<https:/notifications/unsubscribe-auth/AWD56G4NRFHDGYAPIJSJEEDUVNW3JANCNFSM5KR7VO7Q>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Hi Hector,
Also about the current fix in multi_pack.py 282-292:
try:
return self._inverse_pack_ref[pack_id]
except KeyError: # sometimes after deserialization the dict have string as key
s_dict: dict = self._inverse_pack_ref
s_key = str(pack_id)
try:
return s_dict[s_key]
except KeyError as ke: # really not found
raise ProcessExecutionException(
f"Pack {pack_id} is not in this multi-pack."
) from ke
this is becasue after multi-pack deserialization the _inversepack_ref dictionary becomes [str, int] instead of [int, int] . If you send in an int to lookup it will generate keyError. So you might want to have some one to have a look to see why it changes.
(this can be debugged from entry_data_structures_test.py line 174)
Thanks,
James
James Xiao
Architect
T: +971 2 8113163
E: ***@***.***
www.mbzuai.ac.ae<http://www.mbzuai.ac.ae>
Mohamed bin Zayed University of Artificial Intelligence
Masdar City, Building 1B, 3rd Floor
Abu Dhabi, UAE
[cid:MBZUAI_Logo_4d283c4f-1a0d-4aed-8a25-b17ff8b431f8.png]
The world’s first specialized research-based AI university
…________________________________
发件人: James Xiao ***@***.***>
发送时间: 2022年1月13日 18:09
收件人: asyml/forte ***@***.***>
主题: 回复: [asyml/forte] Fix559 (PR #576)
Thanks Hector,
I've just changed to "Fix559 Change reference/access of individual pack in the MultiPack from (array) index based to pack_id based (to facilitate delete, insert), with version-enabled backward compatibility"
Also about today's check in: I've made some changes to 5 places however this might also need a MultiPack change related to deserialization (500/547 related) by Bater tomorrow, so the current commit version is not the fully sync'd / final version.
Thanks,
James
________________________________
发件人: Hector ***@***.***>
发送时间: 2022年1月11日 3:56
收件人: asyml/forte ***@***.***>
抄送: James Xiao ***@***.***>; Author ***@***.***>
主题: Re: [asyml/forte] Fix559 (PR #576)
please provide a description title to the PR
—
Reply to this email directly, view it on GitHub<#576 (comment)>, or unsubscribe<https:/notifications/unsubscribe-auth/AWD56G4NRFHDGYAPIJSJEEDUVNW3JANCNFSM5KR7VO7Q>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Hmm. So I think this is related to the serialization. In fact, JSON requires all keys to be a string. So if we do a Json dump we will get the string. This should be a bug that we overlook. See here for the discussion: https://stackoverflow.com/questions/1450957/pythons-json-module-converts-int-dictionary-keys-to-strings Btw, would you mind creating an issue about this finding? |
|
Sure will do. Thanks Hector for the investigation and quick response!
-- Update: the new issue is now entered as #595
|
…fixing , together with asyml#559 related version (DEFAULT to default_PACK_VERSION)
…fixing , adding # pylint: disable=W0212 for changing _inverse_pack_ref dictionary
…directive to explict (from W0212)
…imited by pylint)
… base_pack (plus additional fix code) without calling super().from_string, as type conversion from base_pack to multi_pack is indeed problematic and not strict (and thus not recommended)
|
Please understand the usage of the |
…ible version variable name change.
…requirements.txt is modified to include packaging as required.
This PR fixes #559. (as the main issue) , and also fixes #595. of a dictionary key (changed form it to str by the deserializer) issue currently only affecting this change (thus needs to be changed/delivered together)
Description of changes
In order to change from using array index number to pack_id (in order to safe delete pack from multi-pack without affecting the reference to other packs), following places are changed:
"_resolve_pointer" of MultiEntry utilizing latest version of pack to perform backward_compatibility check (and resolve the pointer in the old way if the pack is before/oder than the BACKWARD_COMPATIBLE_VER. in add_member() method as well as in the set_parent and set_child of class MultipackLink, pack_id is used instead of pack_index.
(Also includes recent changes in version for pack in base_pack.py and version.py)
The (dependent) fixing of issue595 that correct the dictionary key type in method "deserialize" (from line 647) and "from_string" (line 687) of multi_pack.py, basically change the keys of dictionary _inverse_pack_ref from str back to int
Possible influences of this PR.
Old version of packs such as those being serialized to disk, when loaded to this new version, need to be version checked and perform different ways to resolve the pointer according (old version using index, newer version using pack_id)
Test Conducted
Some adding/deleting of multipack is tested.
Also deserialize related test case "test_multipack_deserialized_dictionary_recover"
is added for the dictionary key type correction (for #595) in "entry_data_structures_test.py"