Test failiure on Python 3.8

[Aniket-Pradhan]

Hello there!

Thanks for working on and providing python-odml. 😄

We were working on packaging this tool for Fedora, and it seems that one of the tests fails on Python 3.8. No worries, it seems to be an issue of PyYaml (ref). As per the team working on PyYaml, this should be resolved in the next stable release.

On the other hand, for now, we can add Loader=yaml.Loader in odmlparser.py#136 to fix the issue. I have checked, and it does not break any tests.

I will send in a quick PR to fix the issue. :D

The error log is here;

self = <test.test_dtypes_integration.TestTypesIntegration testMethod=test_time>

    def test_time(self):
        time_string = '12:34:56'
        time = dt.time(12, 34, 56)
        val_in = time_string
        vals_in = [None, "", time_string, time]
        parent_sec = self.doc.sections[0]
        _ = odml.Property(name="time test single", dtype="time",
                          value=val_in, parent=parent_sec)
        _ = odml.Property(name="time test", dtype="time",
                          value=vals_in, parent=parent_sec)
    
        # Test correct json save and load.
        odml.save(self.doc, self.json_file, "JSON")
        jdoc = odml.load(self.json_file, "JSON")
    
        self.assertEqual(jdoc.sections[0].properties[0].dtype, odml.dtypes.DType.time)
        self.assertIsInstance(jdoc.sections[0].properties[0].values[0], dt.time)
        self.assertEqual(jdoc.sections[0].properties[1].dtype, odml.dtypes.DType.time)
        for val in jdoc.sections[0].properties[1].value:
            self.assertIsInstance(val, dt.time)
        self.assertEqual(jdoc.sections[0].properties[1].values[2], time)
        self.assertEqual(jdoc.sections[0].properties[1].values[3], time)
        self.assertEqual(self.doc, jdoc)
    
        # Test correct xml save and load.
        odml.save(self.doc, self.xml_file)
        xdoc = odml.load(self.xml_file)
    
        self.assertEqual(xdoc.sections[0].properties[0].dtype, odml.dtypes.DType.time)
        self.assertIsInstance(xdoc.sections[0].properties[0].values[0], dt.time)
        self.assertEqual(xdoc.sections[0].properties[1].dtype, odml.dtypes.DType.time)
        for val in xdoc.sections[0].properties[1].value:
            self.assertIsInstance(val, dt.time)
        self.assertEqual(xdoc.sections[0].properties[1].values[2], time)
        self.assertEqual(xdoc.sections[0].properties[1].values[2], time)
        self.assertEqual(self.doc, xdoc)
    
        # Test correct yaml save and load.
        odml.save(self.doc, self.yaml_file, "YAML")
>       ydoc = odml.load(self.yaml_file, "YAML")

test/test_dtypes_integration.py:74: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
odml/fileio.py:21: in load
    return reader.from_file(filename)
odml/tools/odmlparser.py:136: in from_file
    self.parsed_doc = yaml.load(yaml_data)
.venv/lib64/python3.8/site-packages/yaml/__init__.py:114: in load
    return loader.get_single_data()
.venv/lib64/python3.8/site-packages/yaml/constructor.py:43: in get_single_data
    return self.construct_document(node)
.venv/lib64/python3.8/site-packages/yaml/constructor.py:52: in construct_document
    for dummy in generator:
.venv/lib64/python3.8/site-packages/yaml/constructor.py:399: in construct_yaml_seq
    data.extend(self.construct_sequence(node))
.venv/lib64/python3.8/site-packages/yaml/constructor.py:121: in construct_sequence
    return [self.construct_object(child, deep=deep)
.venv/lib64/python3.8/site-packages/yaml/constructor.py:121: in <listcomp>
    return [self.construct_object(child, deep=deep)
.venv/lib64/python3.8/site-packages/yaml/constructor.py:92: in construct_object
    data = constructor(self, node)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <yaml.loader.FullLoader object at 0x7f27eff0a3a0>
node = SequenceNode(tag='tag:yaml.org,2002:python/object/apply:datetime.time', value=[ScalarNode(tag='tag:yaml.org,2002:binary', value='DCI4AAAA\n')])

    def construct_undefined(self, node):
>       raise ConstructorError(None, None,
                "could not determine a constructor for the tag %r" % node.tag,
                node.start_mark)
E       yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:python/object/apply:datetime.time'
E         in "/tmp/tmp_jhz9b1i.odml/test.yaml", line 11, column 9

.venv/lib64/python3.8/site-packages/yaml/constructor.py:418: ConstructorError

[mpsonntag]

hi @Aniket-Pradhan!

thanks a lot for bringing this up and already providing a PR to fix the issue, this is awesome!

it seems that the pyyaml issue you are referencing is quite extensive and I think I want to run some additional tests locally to make sure our current automated tests are not missing anything.

[Aniket-Pradhan]

thanks a lot for bringing this up and already providing a PR to fix the issue, this is awesome!

You're welcome. 😄

it seems that the pyyaml issue you are referencing is quite extensive and I think I want to run some additional tests locally to make sure our current automated tests are not missing anything.

Seems great. If there's anything I can help with, please do tell. I would be happy to help/contribute. :D

[mpsonntag]

Hi @Aniket-Pradhan!

I am not able to reproduce the error. I tried all combinations of the latest release version 1.4.3 and the current master branch with Python 3.8.1 and the odml default pyyaml version 4.2b4 and the latest pyyaml version 5.3.

I would really like to reproduce the error so I can include it into our test matrix and catch similar errors in the future. Could you tell me the specifics of your install? The exact python version and the exact pyyaml version you are using? Thanks a lot!

[Aniket-Pradhan]

Hey!

I had been using the PyYaml version 5.1.2 (the one present in the Fedora repositories). I did not use the default version (4.2b4) because it was a beta release, and was not available in the Fedora repositories. Python version is 3.8.1 and odml's version is 1.4.3.

The issue has been fixed with PyYaml version 5.2 as far as I can see from this (#144) issue.

[mpsonntag]

Alright! I had a stupid mistake in my test matrix, my bad. I started fresh and was able to reproduce the error and was able to verify that the fix works fine with the important combinations of Python (2.7, 3.6, 3.7, 3.8) and pyyaml (4.2b2, 5.1, 5.1.2, 5.2, 5.3) versions.

I think with this we can also drop the pyyaml 4.2b2 dependency, which is very neat.

I still have to test loading from a yaml string and updating the tests for these cases, they are not very extensive.

And finally it seems that datetime.time is the only odml type that is serialized not as a string but as a python object (!!python/object/apply:datetime.time) with a binary value. datetime.datetime and datetime.date are serialized as strings and properly loaded back to the corresponding python objects. If this can be fixed, we can also use a safer yaml Loader since according to the loader deprecation documentation yaml.Loader is the most unsafe one.

@Aniket-Pradhan, thanks again for the issue and the fix!

[Aniket-Pradhan]

And finally it seems that datetime.time is the only odml type that is serialized not as a string but as a python object (!!python/object/apply:datetime.time) with a binary value. datetime.datetime and datetime.date are serialized as strings and properly loaded back to the corresponding python objects. If this can be fixed, we can also use a safer yaml Loader since according to the loader deprecation documentation yaml.Loader is the most unsafe one.

I can try to tinker around and look into this part. I'll get back to you if I make some progress

@Aniket-Pradhan, thanks again for the issue and the fix!

You're welcome ❤️

[Aniket-Pradhan]

Hey! I'm back

I was trying here and there, but I had this doubt, as to why datetime.time is getting serialized as a python object and why datetime.date is not. I could not find a reference where the datetime.date object is explicitly being converted to a string (for YAML). Is it happening internally in PyYaml?

Anyhow, I found a solution. Just like JSONDateTimeSerializer, we can define a function for the YAML dumper so that it converts the datetime.time object to a string before dumping. It can be written like this:

def convert_time_to_str(dumper, data):
    return dumper.represent_scalar('tag:yaml.org,2002:str', str(data))
# "tag:yaml.org,2002:str" is the URI for the string object

yaml.add_representer(datetime.time, convert_time_to_str)

The test passes successfully and seems to be working fine. If you want, I can submit another PR (or add another commit in the previous PR) for this fix.

[mpsonntag]

I also was unsuccessfully trying to figure out why datetime and date get nicely serialized and time was not...

Your datetime.time dumper works like a charm, thanks a lot! If you want to, please add your code as a commit to the open PR, then we'll merge it.

I'll update the yaml string loader with updated test with a follow up PR.

[Aniket-Pradhan]

I also was unsuccessfully trying to figure out why datetime and date get nicely serialized and time was not...

This might be related to the datetime library... I will try to ping those guys to figure something out.

Your datetime.time dumper works like a charm, thanks a lot! If you want to, please add your code as a commit to the open PR, then we'll merge it.

Sure, I will do that by tomorrow. :D

I'll update the yaml string loader with updated test with a follow up PR.

Thanks... :D

[Aniket-Pradhan]

Hello, devs!

Sorry for reviving this ticket again. I was wondering if its possible to make a minor release of the odML library. The main purpose of this ticket was to resolve the errors which came up during packaging the library for Fedora with Py3.8. I can include this commit as a patch with the latest release (1.4.3), but having a new release would make the process very neat.

Also, the last release was a decade ago (Oct. 2019 😛 ), and there have been multiple changes since then. Hence, a minor release with the changes committed would be great.

[mpsonntag]

@Aniket-Pradhan, I am trying to finish some features and do a release until the end of the week if that's not too late for you.

[Aniket-Pradhan]

That's no issue. There are no hurries.

[mpsonntag]

@Aniket-Pradhan I just released a new odml version on PyPI containing the changes.

[Aniket-Pradhan]

Thanks a lot @mpsonntag . I'll get to creating the package right away.