[iterator] Use iter and next to loop through Python Object (TF-337) #23268

sendilkumarn · 2019-03-13T11:57:37Z

This adds iterator support for iterating through the Python Object.

This replaces Mutable collection with the IteratorProtocol and Sequence.

Resolves TF-337.

P.S. This is my first PR.

pschuh · 2019-03-13T19:55:54Z

stdlib/public/Python/Python.swift

Is it necessary to delete this? I think your change should be purely additive.

Interesting, without removing this change It is falling back to the same iteration. I might be doing something wrong here.

In think you would need to implement:
public func makeIterator() -> PythonObject { return Python.iter(self) }
as part of a conformance to Sequence?

Adding Sequence conformance works. Thanks 👍

pschuh · 2019-03-13T20:13:54Z

stdlib/public/Python/Python.swift

Hmm, this is looking simpler:
guard let result: OwnedPyObjectPointer = PyIter_Next(obj.borrowedPyObject) else {
try! throwPythonErrorIfPresent()
return nil
}
return PythonObject(owning: result)

This is exactly what I had initially, but I refrained from the straight C-API calls.

pschuh · 2019-03-13T22:49:15Z

stdlib/public/Python/Python.swift

Clean these up because they aren't used?

pschuh · 2019-03-13T22:50:20Z

stdlib/public/Python/Python.swift

I believe the prevailing syntax is "ClassName : Protocol". Please update to that here and for MutableCollection.

pschuh · 2019-03-13T22:50:33Z

Looking quite close!

stdlib/public/Python/Python.swift

rxwei · 2019-03-14T00:07:24Z

stdlib/public/Python/Python.swift

Is this over 80 columns? If so, can you please make it fit?

rxwei · 2019-03-14T00:08:26Z

stdlib/public/Python/Python.swift

Our local convention is to avoid ‘self.’ unless it’s required.

rxwei · 2019-03-14T00:09:32Z

stdlib/public/Python/Python.swift

Avoid explicit type annotation when it can be inferred.

rxwei · 2019-03-14T00:09:59Z

test/Python/python_runtime.swift

Add a space after each comma.

airspeedswift · 2019-03-14T01:00:35Z

stdlib/public/Python/Python.swift

I recommend against the technique of making sequences their own iterators too. It's usually a better choice to declare a nested Iterator type, ensuring that the sequence is unambiguously multi-pass with a fresh iterator from the start on each call to makeIterator().

I am not sure I get this part correctly

You could make a nested Iterator struct which would wrap a PythonObject and just expose next(). This way, it is more type-safe if someone tries to iterate a python sequence themselves.

You can define a ‘struct Iterator : IteratorProtocol {}’ within a PythonObject extension.

I updated the PR. Is this internal struct okay to be public here?

rxwei · 2019-03-16T06:12:53Z

stdlib/public/Python/Python.swift

This property should be private or fileprivate because it is only used within this extension.

stdlib/public/Python/Python.swift

rxwei · 2019-03-16T06:15:42Z

stdlib/public/Python/Python.swift

Please rename this to something with less abbreviation. How about pythonIterator?

Suggested change

let pyObjIterator: PythonObject

let pythonIterator: PythonObject

sendilkumarn · 2019-03-17T00:00:39Z

Thanks @rxwei 👍 for the comments

rxwei · 2019-03-17T00:51:13Z

stdlib/public/Python/Python.swift

I think this can be simplified to use ‘Python.next()’ directly. Have you tried that?

There’s of course some trade off between efficiency and convenience. I think consistency is key here—we should either use pure C APIs for ‘makeIterator’ and ‘next’, or pure Python APIs.

In fact, that was my first commit. I think it is better to stick with one [edited] APIs, rather than mix and match. #23268 (comment)

Edit: The problem with Python API is that Python.next and iter will return PythonObject rather than PythonObject? This complicates things, because the error is thrown when the method is called dynamically. I will try to use C-Apis for both

The problem with Python API is that Python.next and iter will return PythonObject rather than PythonObject? This complicates things, because the error is thrown when the method is called dynamically.

This is okay--you can check whether the python object is equal to Python.None. However, I like using C APIs for both since that's strictly more efficient :)

rxwei

Thanks for contributing!

rxwei · 2019-03-18T11:22:11Z

stdlib/public/Python/Python.swift

Our local convention is to omit self. unless it's required.

Suggested change

let result = PyObject_GetIter(self.borrowedPyObject)

let result = PyObject_GetIter(borrowedPyObject)

rxwei · 2019-03-18T11:25:36Z

stdlib/public/Python/Python.swift

This could cause a segfault. PythonObject.init(consuming:) already takes ownership of a PyObject pointer and calls Py_DecRef in its deinit. Remove this defer block and then you are good.

Also, there's no need to add empty lines in this compact function.

rxwei · 2019-03-18T11:34:36Z

stdlib/public/Python/Python.swift

If PyObject_GetIter returns nil (aka, a NULL pointer), this would be a precondition failure and a Python TypeError must have been raised, according to PyObject_GetIter's documentation). Instead of force-unwrapping result, we should use a guard statement and throw the Python exception.

guard let result = PyObject_GetIter(borrowedPyObject) else { try! throwPythonErrorIfPresent() // Unreachable. A Python `TypeError` must have been thrown. preconditionFailure() } return Iterator(pythonIterator: PythonObject(consuming: result))

sendilkumarn · 2019-03-18T12:56:31Z

Thanks for the awesome help ( I learnt a lot) 👍

rxwei · 2019-03-18T13:28:41Z

stdlib/public/Python/Python.swift

+        try! throwPythonErrorIfPresent()
+        return nil
+      }
+


One last comment: Could you please remove this empty line?

rxwei · 2019-03-18T14:50:37Z

Thanks a lot, @sendilkumarn!

rxwei · 2019-03-18T14:50:42Z

@swift-ci please test tensorflow

rxwei · 2019-03-18T14:51:12Z

@swift-ci please test tensorflow

rxwei · 2019-03-18T14:51:16Z

@swift-ci please test tensorflow

…wiftlang#23268) This adds iterator support for iterating through a `PythonObject`. Makes `PythonObject` conform to `Sequence`. Resolves [TF-337](https://bugs.swift.org/browse/TF-337).

pschuh reviewed Mar 13, 2019

View reviewed changes

sendilkumarn force-pushed the py-iter branch from 98cf96e to 6c99d72 Compare March 13, 2019 22:41

sendilkumarn changed the title ~~[WIP] [iterator] Use iter and next to loop through Python Object (TF-337)~~ [iterator] Use iter and next to loop through Python Object (TF-337) Mar 13, 2019

pschuh reviewed Mar 13, 2019

View reviewed changes

stdlib/public/Python/Python.swift Outdated

Copy link

Contributor

pschuh Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean these up because they aren't used?

pschuh reviewed Mar 13, 2019

View reviewed changes

sendilkumarn force-pushed the py-iter branch from 6c99d72 to 4d6d953 Compare March 13, 2019 22:57

rxwei reviewed Mar 14, 2019

View reviewed changes

airspeedswift reviewed Mar 14, 2019

View reviewed changes

sendilkumarn force-pushed the py-iter branch 2 times, most recently from a94bd94 to 7785ea5 Compare March 14, 2019 15:38

rxwei reviewed Mar 16, 2019

View reviewed changes

sendilkumarn force-pushed the py-iter branch from 7785ea5 to 3d4976f Compare March 16, 2019 09:36

rxwei reviewed Mar 17, 2019

View reviewed changes

sendilkumarn force-pushed the py-iter branch from 3d4976f to 618e89a Compare March 18, 2019 10:56

rxwei approved these changes Mar 18, 2019

View reviewed changes

[iterator] Use iter and next to loop through Python Object (TF-337)

7929233

sendilkumarn force-pushed the py-iter branch from 93e10f7 to 7929233 Compare March 18, 2019 12:57

rxwei reviewed Mar 18, 2019

View reviewed changes

Update Python.swift

1b6ddb7

rxwei merged commit f612e90 into swiftlang:tensorflow Mar 18, 2019

sendilkumarn deleted the py-iter branch March 18, 2019 15:58

	let pyObjIterator: PythonObject
	let pythonIterator: PythonObject

	let result = PyObject_GetIter(self.borrowedPyObject)
	let result = PyObject_GetIter(borrowedPyObject)

[iterator] Use iter and next to loop through Python Object (TF-337) #23268

[iterator] Use iter and next to loop through Python Object (TF-337) #23268

Uh oh!

Conversation

sendilkumarn commented Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pschuh commented Mar 13, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sendilkumarn commented Mar 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sendilkumarn Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sendilkumarn commented Mar 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei commented Mar 18, 2019

Uh oh!

rxwei commented Mar 18, 2019

Uh oh!

rxwei Mar 14, 2019 •

edited

Loading

sendilkumarn Mar 17, 2019 •

edited

Loading