detect breaking changes

[sungwy]

Implement #334

• Uses griffe to detect breaking changes since the latest release
• Allows users to specify an exclusion list that will silence test failures on the introduction of intended breaking changes

[sungwy]

The CI currently reports 58 breaking changes between 0.5.1 and 976c0ea. (I'm trying to understand in which PR these breaking changes were introduced)

When test_api.py was first written, it detected just 1 breaking change related to the backward incompatible Daft change:

Example stack trace:

assert not [{'kind': <BreakageKind.ATTRIBUTE_CHANGED_VALUE: 'Attribute value was changed'>, 'new_value': ExprDict(keys=['1', '2'], values=[ExprCall(function=ExprName(name='StructType', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[ExprCall(function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[ExprKeyword(name='field_id', value='100', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='name', value="'file_path'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='field_type', value=ExprCall(function=ExprName(name='StringType', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[]), function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='required', value='True', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='doc', value="'Location URI with FS scheme'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))))]), ExprCall(function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[ExprKeyword(name='field_id', value='101', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='name', value="'file_format'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='field_type', value=ExprCall(function=ExprName(name='StringType', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[]), function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='required', value='True', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='doc', value="'File format name: avro, orc, or parquet'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))))]), ExprCall(function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[ExprKeyword(name='field_id', value='102', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='name', value="'partition'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='field_type', value=ExprCall(function=ExprName(name='StructType', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[]), function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='required', value='True', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='doc', value="'Partition data tuple, schema based on the partition spec'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))))]), ExprCall(function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py'))), arguments=[ExprKeyword(name='field_id', value='103', function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='name', value="'record_count'", function=ExprName(name='NestedField', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD/pyiceberg/manifest.py')))), ExprKeyword(name='field_type', value=ExprCall(function=ExprName(name='LongType', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-1b_iv7st/griffe_HEAD

[sungwy]

Which of these breaking change types should we include in our unit test?

I've currently removed ATTRIBUTE_CHANGED_VALUE type because it was creating a lot of noise in the output.

Example:

{'kind': <BreakageKind.ATTRIBUTE_CHANGED_VALUE: 'Attribute value was changed'>, 'object_path': 'pyiceberg.catalog.hive.DEFAULT_PROPERTIES', 'old_value': ExprDict(keys=["'write.parquet.compression-codec'"], values=["'zstd'"]), 'new_value': ExprDict(keys=[ExprAttribute(values=[ExprName(name='TableProperties', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-v_k3_7xc/griffe_HEAD/pyiceberg/catalog/hive.py'))), ExprName(name='PARQUET_COMPRESSION', parent=ExprName(name='TableProperties', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-v_k3_7xc/griffe_HEAD/pyiceberg/catalog/hive.py'))))])], values=[ExprAttribute(values=[ExprName(name='TableProperties', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-v_k3_7xc/griffe_HEAD/pyiceberg/catalog/hive.py'))), ExprName(name='PARQUET_COMPRESSION_DEFAULT', parent=ExprName(name='TableProperties', parent=Module(PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-v_k3_7xc/griffe_HEAD/pyiceberg/catalog/hive.py'))))])])}

[sungwy]

Breakage Kind	Description	Ignore	Rationale
PARAMETER_MOVED	Positional parameter was moved
PARAMETER_REMOVED	Parameter was removed
PARAMETER_CHANGED_KIND	Parameter kind was changed
PARAMETER_CHANGED_DEFAULT	Parameter default was changed
PARAMETER_CHANGED_REQUIRED	Parameter is now required
PARAMETER_ADDED_REQUIRED	Parameter was added as required
RETURN_CHANGED_TYPE	Return types are incompatible
OBJECT_REMOVED	Public object was removed
OBJECT_CHANGED_KIND	Public object points to a different kind of object
ATTRIBUTE_CHANGED_TYPE	Attribute types are incompatible
ATTRIBUTE_CHANGED_VALUE	Attribute value was changed		Too noisy, and in most cases not a serious breaking change (like changing a constant value for Table Property)
CLASS_REMOVED_BASE	Base class was removed

[Fokko]

Thanks for setting this up @syun64. This looks great. I think we can just give it a try after the 0.6.0 release and see how noisy it is.

[Fokko]

@syun64 I was on a merging spree, can you rebase once more? 😓

[Fokko]

Looks like we broke something already 😸 Can we make a list to allow breaking changes? Similar to https://github.com/apache/parquet-mr/blob/d8396086b3e3fefc6829f8640917c3bbde0fa9c4/pom.xml#L581-L606

[sungwy]

Looks like we broke something already 😸 Can we make a list to allow breaking changes? Similar to https://github.com/apache/parquet-mr/blob/d8396086b3e3fefc6829f8640917c3bbde0fa9c4/pom.xml#L581-L606

Sure! Thank you for the suggestion. I took a stab at implementing this with a YAML file - let me know what you think!

[Fokko]

This one seems correct, but not exactly what I expected: fd9dc88#diff-23e8153e0fd497a9212215bd2067068f3b56fa071770c7ef326db3d3d03cee9bL89

[sungwy]

Yeah, that's interesting that it treats imports from other modules to be public objects as well

[Fokko]

:)

[Fokko]

I'm pretty sure that the Cython stuff didn't change.

[sungwy]

This is getting pretty interesting!

As you mentioned, from the diff it looks like CythonBinaryDecoder wasn't updated, but when I run help() on pyiceberg.avro.decoder_fast.CythonBinaryDecoder, I'm loading:

class CythonBinaryDecoder(builtins.object)
 |  Implement a BinaryDecoder that reads from an in-memory buffer.
 |  
 |  Methods defined here:
 |  
 |  __reduce__ = __reduce_cython__(...)
 |  
 |  __setstate__ = __setstate_cython__(...)
 |  
 |  read(self, n: 'int')
 |      Read n bytes.
 |  
 |  read_boolean(self) -> 'bool'
 |      Reads a value from the stream as a boolean.
 |      
 |      A boolean is written as a single byte
 |      whose value is either 0 (false) or 1 (true).
 |  
 |  read_bytes(self)
 |      Bytes are encoded as a long followed by that many bytes of data.
 |  
 |  read_double(self)
 |      Reads a value from the stream as a double.
 |      
 |      A double is written as 8 bytes.
 |      The double is converted into a 64-bit integer using a method equivalent to
 |      Java's doubleToLongBits and then encoded in little-endian format.
 |  
 |  read_float(self)
 |      Reads a value from the stream as a float.
 |      
 |      A float is written as 4 bytes.
 |      The float is converted into a 32-bit integer using a method equivalent to
 |      Java's floatToIntBits and then encoded in little-endian format.
...

Which might explain why methods like read_double, read_float are being reported to having their types changed, whereas read_boolean is being reported to having maintained the same interface.

Maybe there was a change that made Griffe take this definition into account, and it wasn't prior to that

[sungwy]

Alright, so it looks like this false positive is being generated because we are using two separate approaches for loading the Package in griffe. We use load_git to load pyiceberg's latest release directly from Git, and then we use load to load the local implementation of pyiceberg with the coder's current change. Tracking the outputs of CythonBinaryDecoder for the two methods yields different interpretations of the Cython class's interface:

>>> current_git = griffe.load_git("pyiceberg")
>>> current_git['avro']['decoder_fast']['CythonBinaryDecoder'].relative_filepath
PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-v1wuhe7z/griffe_HEAD/pyiceberg/avro/decoder_fast.pyi')
>>> current_git.all_members['avro']['decoder_fast']['CythonBinaryDecoder'].all_members
{'__init__': Function('__init__', 21, 22), 'tell': Function('tell', 24, 25), 'read': Function('read', 27, 28), 'read_boolean': Function('read_boolean', 30, 31), 'read_int': Function('read_int', 33, 34), 'read_ints': Function('read_ints', 36, 37), 'read_int_bytes_dict': Function('read_int_bytes_dict', 39, 40), 'read_bytes': Function('read_bytes', 42, 43), 'read_float': Function('read_float', 45, 46), 'read_double': Function('read_double', 48, 49), 'read_utf8': Function('read_utf8', 51, 52), 'skip': Function('skip', 54, 55), 'skip_int': Function('skip_int', 57, 58), 'skip_boolean': Function('skip_boolean', 60, 61), 'skip_float': Function('skip_float', 63, 64), 'skip_double': Function('skip_double', 66, 67), 'skip_bytes': Function('skip_bytes', 69, 70), 'skip_utf8': Function('skip_utf8', 72, 73)}
>>> current_local = griffe.load("pyiceberg")
>>> current_local['avro']['decoder_fast']['CythonBinaryDecoder'].relative_filepath
PosixPath('pyiceberg/avro/decoder_fast.cpython-38-x86_64-linux-gnu.so')
>>> current_local.all_members['avro']['decoder_fast']['CythonBinaryDecoder'].all_members
{'__doc__': Attribute('__doc__', None, None), '__new__': Function('__new__', None, None), '__pyx_vtable__': Attribute('__pyx_vtable__', None, None), '__reduce__': Function('__reduce__', None, None), '__setstate__': Function('__setstate__', None, None), 'read': Function('read', None, None), 'read_boolean': Function('read_boolean', None, None), 'read_bytes': Function('read_bytes', None, None), 'read_double': Function('read_double', None, None), 'read_float': Function('read_float', None, None), 'read_int': Function('read_int', None, None), 'read_int_bytes_dict': Function('read_int_bytes_dict', None, None), 'read_ints': Function('read_ints', None, None), 'read_utf8': Function('read_utf8', None, None), 'skip': Function('skip', None, None), 'skip_boolean': Function('skip_boolean', None, None), 'skip_bytes': Function('skip_bytes', None, None), 'skip_double': Function('skip_double', None, None), 'skip_float': Function('skip_float', None, None), 'skip_int': Function('skip_int', None, None), 'skip_utf8': Function('skip_utf8', None, None), 'tell': Function('tell', None, None)}

So it looks like only the local load is reading the .so, whereas load_git is reading the pyi file.

Maybe we exclude cython APIs from the test while we figure out how to resolve this issue? @Fokko

[Fokko]

Hey @syun64 Thanks for adding the yaml, that looks neat. What's your gist of Griffe? It looks like there are already some false positives.

[sungwy]

Hey @syun64 Thanks for adding the yaml, that looks neat. What's your gist of Griffe? It looks like there are already some false positives.

Thank you for all the feedback @Fokko ! My general impression with griffe is that it's working pretty well, and after carefully reviewing and adding even more breaking changes we've introduced since the last release, I feel convinced that we should include some implementation of this tool.

There are some edge cases that I thought were worth summarizing:

1. Cython Class interface inference isn't perfect. As I noted in this comment, either the .so or .pyi files are being used. And when .so file is used to interprete the interface, it returns None return types for all of its members:

>>> griffe.load("pyiceberg")['avro']['decoder_fast']['CythonBinaryDecoder'].relative_filepath
PosixPath('pyiceberg/avro/decoder_fast.cpython-38-x86_64-linux-gnu.so')
>>> griffe.load("pyiceberg")['avro']['decoder_fast']['CythonBinaryDecoder']['tell'].returns
>>> griffe.load_git("pyiceberg")['avro']['decoder_fast']['CythonBinaryDecoder'].relative_filepath
PosixPath('/tmp/griffe-worktree-iceberg-python-HEAD-nu9f66g2/griffe_HEAD/pyiceberg/avro/decoder_fast.pyi')
>>> griffe.load_git("pyiceberg")['avro']['decoder_fast']['CythonBinaryDecoder']['tell'].returns
ExprName(name='int', parent=Class('CythonBinaryDecoder', 20, 73))

2. Import removal is also considered as a breaking change because it's a public function/Class that was available on the module, that's been removed.

Outside of these two edge cases, I think all the exclusions listed are very accurate, and I think the test will require the contributors to think twice about making a breaking change, or to document an intended breaking change into the yaml file so we developers are able to trace down breaking changes a lot easier.

Current pyiceberg-0.6.0 exclusion list:

exclude:
- pyiceberg.table.create_mapping_from_schema # Removed import (Alias) #441
- pyiceberg.table.Table.next_sequence_number # Table -> TableMetadata #471
- pyiceberg.table.Table.new_snapshot_id # Table -> TableMetadata #471
- pyiceberg.table.StaticTable.next_sequence_number # Table -> TableMetadata #471
- pyiceberg.table.StaticTable.new_snapshot_id # Table -> TableMetadata #471
- pyiceberg.table.Transaction.add_snapshot # removed in favor of directly using TableUpdate and TableRequirement #471
- pyiceberg.table.Transaction.set_ref_snapshot # removed in favor of directly using TableUpdate and TableRequirement #471
- pyiceberg.catalog.rest.TokenResponse.* # nullability change #466
- pyiceberg.io.pyarrow.write_file # Parameter change #471
- pyiceberg.cli.console.run # Positional parameter change #472

[sungwy]

@jaychia 'suggestion -> reorganize modules to the top level

[sungwy]

@Fokko @HonahX @jaychia - should we remove this from the 0.7.0 milestone? I think we'd benefit from a prolonged discussion and attempt in organizing our API. This feels like an item we just need to complete before the 1.0.0 release