Fix get_file failed when enabling child callback to regard size as a attribute

[OneSizeFitsQuorum]

Allow getattr(f1, "size", None) returns an int. So that we can set size properly here

[martindurant]

This is maybe OK, but I didn't understand the original issue.

Certainly, this PR should add documentation about how to use this change, and a test showing it in practice.

[OneSizeFitsQuorum]

Thanks a lot for reviewing this! Maybe we can change this line from callback.set_size(getattr(f1, "size", None)) to callback.set_size(getattr(f1, "size", None)()) in here.
or add a wrapper function to ensure the parameters passed to set_size is a int, then we do not need update set_size(). What's your opinion?

[OneSizeFitsQuorum]

@martindurant hi, could you please review this again~

[martindurant]

I guess this is fine, but I still don't know why size would be a function: can you make a test case that hits this extra line?

[OneSizeFitsQuorum]

@martindurant HadoopFileSystem inherits from ArrowFSWrapper, and ArrowFSWrapper inherits from AbstractFileSystem. However, AbstractFileSystem does not have a size field, only a size function. Therefore, when you use getattr(f, "size", None) to retrieve it, you get a callable function rather than a field. You need to invoke the function to obtain the corresponding size value.

[martindurant]

Yes, agreed that size is a method on filesystems, .size(path)->int; I think the .size you are after is an attribute on a file object, though - no? e.g., https://github.com/fsspec/filesystem_spec/blob/master/fsspec/spec.py#L1920 .

[OneSizeFitsQuorum]

@martindurant Yes! AbstractBufferedFile has a size attribute, but HadoopFileSystem inherits from ArrowFSWrapper, and ArrowFSWrapper inherits from AbstractFileSystem. So HadoopFileSystem only has a size method rather than an attribute. That's why I encounter this error and submit this pr~

[martindurant]

Why can't we get() the file just because it's not seekable? Isn't that exactly what the code before this PR was doing?

[OneSizeFitsQuorum]

Because we call size() inside the get_file()

[martindurant]

But we can read a stream until it's done. We don't want that workflow to become unusable.

[martindurant]

OK, finally I understand your problem. Our open methods return fsspec.implementations.arrow.ArrowFile . These defer to arrow for various methods including size. And the arrow definition of size ( https://arrow.apache.org/docs/python/generated/pyarrow.NativeFile.html#pyarrow.NativeFile.size ) is a method not an attribute.

So what we actually need to do is, rather than change the callback mechanism, make size a property:

    @property
    def size(self)
        return self.stream.size()

and actually this gives a chance to intercept too - are there cases where we already know the size rather than having to calculate it again?

[OneSizeFitsQuorum]

@martindurant
Apologies for the unclear wording earlier, which caused you to spend extra time, but fortunately, You now fully understand my point, and I respect you for your patience!

So, if I understand correctly, you're suggesting that we should submit a PR to pyarrow to add a size attribute? However, there are two concerns:

• This would introduce a variable and a method with the same name in pyarrow, which is generally not best practice and could lead to confusion.
• I'm unsure if other fsspec implementations also use size as a method. If that’s the case, making a more generalized change in fsspec seems like the most cost-effective solution, and it would also improve the adaptability of fsspec.

What’s your opinion on this?

[martindurant]

No, there is no need to change arrow - the class is here: https://github.com/fsspec/filesystem_spec/blob/master/fsspec/implementations/arrow.py#L230

[OneSizeFitsQuorum]

@martindurant Yes, you've found the most elegant solution! I found that for the get function, once the callback passed is not the default default lookback but rather a tqdmcallback with some actual behavior, it triggers an error. After setting size as a property, I don't see any more issues

BTW, the left ci failure seems irrelevant with this pr, as I comment out all my changes in this commit, but the ci still fails

[martindurant]

Can you please remove "size" from the list at L226