Add support for BigInteger in CPython engine by mmisol · Pull Request #10830 · DynamoDS/Dynamo

mmisol · 2020-06-30T20:04:11Z

Purpose

Support is added for encoding decoding BigInteger values from Python
int that have a size that exceeds what can fit in a long.

Also a BigInteger encoder/decoder is added to the custom encoders of
Python.NET in order to support encoding and decoding in the context of
function calls.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

@DynamoDS/dynamo

mjkkirschner · 2020-06-30T20:38:35Z

@mmisol 😢

(CoreCompile target) -> 
  Encoders\BigIntegerEncoder.cs(23,25): error CS8107: Feature 'default literal' is not available in C# 7.0. Please use language version 7.1 or greater. [C:\projects\dynamo\src\Libraries\DSCPython\DSCPython.csproj]
    2057 Warning(s)
    1 Error(s)
Time Elapsed 00:01:17.13

maybe time to retire this build and use our github action instead?

mmisol · 2020-06-30T20:40:33Z

Yeah. I saw that appveyor thing probably needs an update, but it's not even ours! Probably it is a good time to retire it. I read we should just remove the web hook and that's it.

mjkkirschner · 2020-06-30T20:57:55Z

src/Libraries/DSCPython/CPythonEvaluator.cs

+        /// </summary>
+        private static void InitializeEncoders()
+        {
+            var encoders = new IPyObjectEncoder[] { new BigIntegerEncoder() };


@aparajit-pratap these are the encoders I mentioned before.

@mmisol was the biginteger encoder/decoder added by you? I may have missed it in your PR's to Python.NET repo.

BigIntegerEncoder is part of this submission. The API to add them already existed. One thing I did add in Python.NET is the function PyLong.ToBigInteger, which is used by the decoder.

mjkkirschner · 2020-06-30T20:58:23Z

src/Libraries/DSCPython/Encoders/BigIntegerEncoder.cs

+
+namespace DSCPython.Encoders
+{
+    internal class BigIntegerEncoder : IPyObjectEncoder, IPyObjectDecoder


thank you for making this internal!
🎉

test/Libraries/DynamoPythonTests/PythonEvalTestsWithLibraries.cs

mjkkirschner · 2020-06-30T20:59:39Z

test/Libraries/DynamoPythonTests/IronPythonTests.csproj

    </ProjectReference>
+    <ProjectReference Include="..\..\..\src\Tools\DynamoShapeManager\DynamoShapeManager.csproj">
+      <Project>{263fa9c1-f81e-4a8e-95e0-8cdae20f177b}</Project>
+      <Name>DynamoShapeManager</Name>


just curious about the requirement for these dependencies.

So, when you inherit from DynamoModelTestBase and want to customize CreateStartConfiguration you pass some parameters that will complain because you don't have these.

ahh, okay, generally I would say avoid loading libraries that we don't need during the test as these types will be imported into the VM over and over again for each test which can be kind of slow. So if we write 1000 ironPython tests we'll load all the geometry types into the VM foreach test.

aparajit-pratap · 2020-06-30T21:03:01Z

Why are we spending a whole bunch of time with BigIntegers. They just happen to be supported by IronPython but not a big use case for zero touch nodes or even python nodes for that matter.

mjkkirschner · 2020-07-01T01:18:01Z

src/Libraries/DSCPython/Encoders/BigIntegerEncoder.cs

+
+            using (var pyLong = PyLong.AsLong(pyObj))
+            {
+                value = (T)(object)pyLong.ToBigInteger();


staring at this I'm assuming that PythonNet will only ever call this method when T is bigInteger ?

Yes. I think there is something strange with the definition of the interface because it's not a generic one but one of its methods is generic. I didn't know how to handle that exactly but I took this from an example they have in a test.

mmisol · 2020-07-02T13:03:05Z

Hi @mjkkirschner @aparajit-pratap . If there is nothing else missing here, can I ask for your approval? Thanks

aparajit-pratap · 2020-07-02T13:48:08Z

src/Libraries/DSCPython/CPythonEvaluator.cs

                                }
-                                return dict;
                            }
                            else if (PyLong.IsLongType(pyObj))


aparajit-pratap · 2020-07-02T13:48:37Z

src/Libraries/DSCPython/CPythonEvaluator.cs

                                }
-                                return list;
                            }
                            else if (PyDict.IsDictType(pyObj))


No need for else here.

aparajit-pratap · 2020-07-02T13:50:06Z

src/Libraries/DSCPython/CPythonEvaluator.cs

+                                    {
+                                        return pyLong.ToInt64();
+                                    }
+                                    catch (PythonException exc) when (exc.Message.StartsWith("OverflowError"))


Interesting, I haven't seen a when clause like this before.

These are called exception filters. They allow to catch an exception type conditionally to the fact they also match the expression given in when. They were introduced in C# 6 https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-6#exception-filters

aparajit-pratap

Minor comments; other than that LGTM.

mmisol added 7 commits June 29, 2020 12:00

WIP: missing binary and uncomment test

6f3bdad

Support conversion from/to BigInteger

e06d132

Undo binary change

1f13ffa

Add test

55aa3fe

Merge branch 'master' into cpython_biginteger

dfdd0ef

Fix test

da919ac

Also fix original failing test

f7bac24

mmisol requested a review from a team June 30, 2020 20:04

mjkkirschner reviewed Jun 30, 2020

View reviewed changes

test/Libraries/DynamoPythonTests/PythonEvalTestsWithLibraries.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jun 30, 2020

View reviewed changes

Remove ProtoGeometry in test

cd5cfc3

mjkkirschner reviewed Jul 1, 2020

View reviewed changes

aparajit-pratap reviewed Jul 2, 2020

View reviewed changes

aparajit-pratap approved these changes Jul 2, 2020

View reviewed changes

Remove unneeded else statements

b34c027

QilongTang approved these changes Jul 2, 2020

View reviewed changes

Use FFITarget instead of new zero-touch

b3be3f5

mmisol merged commit af43ba9 into DynamoDS:master Jul 2, 2020

reddyashish mentioned this pull request Jul 8, 2020

[DYN-2797] Provide custom node info in the IronPython2 migration dialog. #10862

Merged

7 tasks

Conversation

mmisol commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

Uh oh!

mjkkirschner commented Jun 30, 2020

Uh oh!

mmisol commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Jun 30, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmisol commented Jul 2, 2020

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!

aparajit-pratap left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mmisol commented Jun 30, 2020 •

edited

Loading

aparajit-pratap Jun 30, 2020 •

edited

Loading