[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

singlis · 2018-09-12T23:37:45Z

The current state did not handle serializing out a list when a list was
set in the configuration. This now adds that functionality along with
unit tests.

JIRA: REEF-2044

Closes #

The current state did not handle serializing out a list when a list was set in the configuration. This now adds that functionality along with unit tests. JIRA: [REEF-2044](https://issues.apache.org/jira/browse/REEF-2044) Closes #

singlis · 2018-09-19T20:52:31Z

@jwang98052 - Hello Julia, would you be able to take a look? I believe this is an area you are familiar with. #WontFix

singlis · 2018-10-03T18:05:52Z

@jwang98052 - have you had a chance to look at these changes? Thanks! #WontFix

markusweimer · 2018-10-09T16:44:17Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+                .BindList<ListOfStrings2, string>(GenericType<ListOfStrings2>.Class, stringList2)
+                .Build();
+
+            ConfigurationFile.WriteConfigurationFile(conf, "ListOfStringsConf.txt");


This file should be deleted by the end of the tests. Or, better yet, serialize the configuration to a byte[] and back. #Resolved

This is not needed -- I actually convert the config to a string and then read back in the string. So I have deleted this line.

In reply to: 223778711 [](ancestors = 223778711)

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

markusweimer · 2018-10-09T16:46:02Z

lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs

@@ -260,6 +260,28 @@ public AvroConfiguration ToAvroConfiguration(IConfiguration c)
                l.Add(new ConfigurationEntry(e.Key.GetFullName(), val));
            }

+            IEnumerator bl = conf.GetBoundList().GetEnumerator();
+            while (bl.MoveNext())


why not a for each loop? #Resolved

What is the expected behavior for an empty list? The code does run - however nothing is written out because there is nothing to serialize. If that is expected, I will modify the test accordingly.

In reply to: 223779318 [](ancestors = 223779318)

Yes, I believe that is the expected behavior. #Resolved

markusweimer · 2018-10-09T16:46:43Z

lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs

+                    }
+                    else
+                    {
+                        Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);


please don't use the Exceptions class anymore. Instead, just throw the exception. #Resolved

markusweimer · 2018-10-09T16:47:14Z

lang/cs/Org.Apache.REEF.Tang/Formats/ConfigurationFile.cs

+                    }
+                    else
+                    {
+                        Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);


Same as above: Replace with throw. Also, add a message to the exception. #Resolved

markusweimer · 2018-10-09T16:47:52Z

lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationImpl.cs

 using Org.Apache.REEF.Tang.Interface;
 using Org.Apache.REEF.Tang.Types;
+using System.Collections.Generic;

 namespace Org.Apache.REEF.Tang.Implementations.Configuration


Changes in this file seem superficial only. Maybe revert? #Resolved

It was sorting the using - but there are no additional changes so I dont think its justified. I will revert.

In reply to: 223779942 [](ancestors = 223779942)

markusweimer · 2018-10-09T16:48:14Z

lang/cs/Org.Apache.REEF.Tang/Interface/IConfiguration.cs

@@ -39,6 +39,7 @@ public interface IConfiguration
        ICollection<IClassNode> GetLegacyConstructors();

        IEnumerator<KeyValuePair<INamedParameterNode, object>> GetBoundSets();
+


Changes in this file are superficial only. Please revert. #Resolved

singlis · 2018-10-19T00:39:48Z

Thanks for the feedback @markusweimer. I have posted an update. Although there is one issue that bothers me with lists which is no code seems to serialize a list of classes, it looks to always be a string.

Is serializing out a list of classes that can be injected supposed to work?

I started to make changes to this to see what it would take. First I had to modify this signature:

ICsConfigurationBuilder BindList<U, V, T>(GenericType<U> iface, IList<GenericType<V>> impl)

to

ICsConfigurationBuilder BindList<U, V, T>(GenericType<U> iface, IList<V> impl)

Then I added a test similar to what is done with Sets:

ICsConfigurationBuilder cb = TangFactory.GetTang().NewConfigurationBuilder();
var t = new System.Collections.Generic.List<Integer1>();
t.Add(new Integer1(2));
t.Add(new Integer1(3));
cb.BindList<ListOfClasses, Integer1, INumber>(GenericType<ListOfClasses>.Class, t)

IInjector i = TangFactory.GetTang().NewInjector(cb.Build());
IList<INumber> actual = i.GetInstance<PoolList>().Numbers;

The error is on the last line when we try to call GetInstance to construct the object. It looks like it has to do with resolving the parameters (2,3) to pass to the class on construction. Note PoolList is a class that has an inject constructor. It takes in IList as the parameter, it has a property called Numbers that returns an IList of INumbers:

            [Inject]
            private PoolList([Parameter(typeof(ListOfClasses))] IList<INumber> numbers)

markusweimer

LGTM. Will run tests and merge, when they pass.

markusweimer · 2018-10-19T17:05:11Z

Is serializing out a list of classes that can be injected supposed to work?

I don't think so. Tang enforces singleton semantics for objects to be instantiated by it. And in a List, one can refer to the same type multiple times, without giving it a name as in NamedParameter. Hence, I believe it is expected behavior for this to only ever work with Sets.

markusweimer

I actually see relevant test failures:

Test Name:	Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestListSerializeEmptyStrings
Test FullName:	Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestListSerializeEmptyStrings
Test Source:	C:\src\reef\lang\cs\Org.Apache.REEF.Tang.Tests\Configuration\TestConfiguration.cs : line 567
Test Outcome:	Failed
Test Duration:	0:00:00.002

Result StackTrace:	
at Org.Apache.REEF.Tang.Implementations.Configuration.CsConfigurationBuilderImpl.Org.Apache.REEF.Tang.Interface.ICsInternalConfigurationBuilder.BindList(Type iface, IList`1 implList) in C:\src\reef\lang\cs\Org.Apache.REEF.Tang\Implementations\Configuration\CsConfigurationBuilderImpl.cs:line 408
   at Org.Apache.REEF.Tang.Implementations.Configuration.CsConfigurationBuilderImpl.BindList[U,T](GenericType`1 iface, IList`1 impl) in C:\src\reef\lang\cs\Org.Apache.REEF.Tang\Implementations\Configuration\CsConfigurationBuilderImpl.cs:line 234
   at Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestSerializeListHelper(IList`1 items, Int32 expectedLists) in C:\src\reef\lang\cs\Org.Apache.REEF.Tang.Tests\Configuration\TestConfiguration.cs:line 513
   at Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestListSerializeEmptyStrings() in C:\src\reef\lang\cs\Org.Apache.REEF.Tang.Tests\Configuration\TestConfiguration.cs:line 573
Result Message:	Org.Apache.REEF.Tang.Exceptions.BindException : BindList failed to bind for ListOfStrings, reason: List cannot contain string that are null or empty

and

Test Name:	Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestListSerializeNullStringValue
Test FullName:	Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestListSerializeNullStringValue
Test Source:	C:\src\reef\lang\cs\Org.Apache.REEF.Tang.Tests\Configuration\TestConfiguration.cs : line 538
Test Outcome:	Failed
Test Duration:	0:00:00.001

Result StackTrace:	
at Org.Apache.REEF.Tang.Implementations.Configuration.CsConfigurationBuilderImpl.Org.Apache.REEF.Tang.Interface.ICsInternalConfigurationBuilder.BindList(Type iface, IList`1 implList) in C:\src\reef\lang\cs\Org.Apache.REEF.Tang\Implementations\Configuration\CsConfigurationBuilderImpl.cs:line 408
   at Org.Apache.REEF.Tang.Implementations.Configuration.CsConfigurationBuilderImpl.BindList[U,T](GenericType`1 iface, IList`1 impl) in C:\src\reef\lang\cs\Org.Apache.REEF.Tang\Implementations\Configuration\CsConfigurationBuilderImpl.cs:line 234
   at Org.Apache.REEF.Tang.Tests.Configuration.TestConfiguration.TestListSerializeNullStringValue() in C:\src\reef\lang\cs\Org.Apache.REEF.Tang.Tests\Configuration\TestConfiguration.cs:line 546
Result Message:	Org.Apache.REEF.Tang.Exceptions.BindException : BindList failed to bind for ListOfStrings, reason: List cannot contain string that are null or empty

…ty lists

jwang98052 · 2018-10-19T18:57:40Z

Sorry for late response. I will have a quick look today.

jwang98052 · 2018-10-19T22:12:08Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+        public void TestListConfigWithEmptyString()
+        {
+            IList<string> stringList1 = new List<string>();
+            stringList1.Add("");


check the behavior of set for empty string to make the behavior consistent.

jwang98052 · 2018-10-19T22:12:23Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+        public void TestListConfigWithNullStringValue()
+        {
+            IList<string> stringList1 = new List<string>();
+            stringList1.Add(null);


Same for null

jwang98052 · 2018-10-19T22:15:12Z

lang/cs/Org.Apache.REEF.Tang/Formats/ConfigurationFile.cs

+                    {
+                        val = GetFullName((string)item);
+                    }
+                    else if (kvp.Value is INode)


You can try a test case for List of instances from different subclasses of the same interface, similar to Set to see if this scenario is working.

motus

Very useful functionality! Will post more comments soon.

motus · 2019-01-05T01:48:54Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+        {
+            IList<string> stringList1 = new List<string>();
+            stringList1.Add("foo");
+            stringList1.Add("bar");


Here and everywhere: can be simply

var stringList1 = new List<string> { "foo", "bar" };

motus · 2019-01-05T01:54:10Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+            IConfiguration conf2 = ConfigurationFile.GetConfiguration(s);
+
+            ListInjectTest injectTest = (ListInjectTest)TangFactory.GetTang().
+                    NewInjector(conf2).GetInstance(typeof(ListInjectTest));


No need to cast - simply write

var injectTest = TangFactory.GetTang().NewInjector(conf2).GetInstance<ListInjectTest>();

motus · 2019-01-05T02:07:13Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+                return;
+            }
+
+            Assert.True(false, "Failed to throw expected exception.");


Here and everywhere: the whole test can be just

Assert.Throws<BindException>(() => TangFactory.GetTang().NewConfigurationBuilder() .BindList<ListOfStrings, string>(new List<string>() { null }) .Build());

motus · 2019-01-05T02:08:08Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

@@ -448,6 +587,7 @@ public void TestSetConfig()
            Assert.True(actual.Contains("six"));
        }

+


why an extra empty line?

motus · 2019-01-05T02:09:33Z

lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs

@@ -254,12 +254,32 @@ public AvroConfiguration ToAvroConfiguration(IConfiguration c)
                }
                else
                {
-                    Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);
+                    throw new TangApplicationException("Unable to serialize set of type {e.Value.GetType()}");


Forgot to use $"" for interpolation.

motus · 2019-01-05T02:10:19Z

lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs

+                    }
+                    else
+                    {
+                        throw new TangApplicationException("Unable to serialize list of type {item.GetType()}");


same as above - use $"". please check for other places as well

motus · 2019-01-05T02:11:11Z

lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/CsConfigurationBuilderImpl.cs

+            {
+                BindList((INamedParameterNode)n, implList);
+            }
+            catch(ArgumentException ex)


add space before ( - and maybe use formatting tool like CodeMaid or similar?

motus

OK I am done with the first pass. Left a few minor nit picks

motus · 2019-01-07T23:17:16Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+            IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
+                .BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList1)
+                .BindList<ListOfStrings2, string>(GenericType<ListOfStrings2>.Class, stringList2)
+                .Build();


No need for the first argument in BindList(): that information is already passed as a generic parameter. can simply write

IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder() .BindList<ListOfStrings, string>(stringList1) .BindList<ListOfStrings2, string>(stringList2) .Build();

(also, maybe define static ITang Tang = TangFactory.GetTang(); that can be used in all tests?)

motus · 2019-01-07T23:22:33Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+                return;
+            }
+
+            Assert.True(false, "Failed to throw expected exception.");


same as above: the entire test can be written as

Assert.Throws<BindException>(() => Tang.NewConfigurationBuilder() .BindList<ListOfStrings, string>(new List<string>() { "" }) .Build());

motus · 2019-01-07T23:25:29Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+        private void TestSerializeListHelper(IList<string> items, int expectedLists = 1)
+        {
+            IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
+                    .BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, items)


here and everywhere: no need to pass GenericType<ListOfStrings>.Class explicitly

motus · 2019-01-07T23:28:28Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+            IList<string> stringList = new List<string>();
+            stringList.Add("foo");
+            stringList.Add("bar");
+            TestSerializeListHelper(stringList);


just

TestSerializeListHelper(new List<string> { "foo", "bar" });

motus · 2019-01-07T23:28:56Z

lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs

+        [Fact]
+        public void TestListSerializeNullStringValue()
+        {
+            string msg = null;


msg is not used anywhere

motus · 2019-01-08T00:56:25Z

lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs

+                    }
+                    l.Add(new ConfigurationEntry(kvp.Key.GetFullName(), val));
+                }
+            }


Also, maybe the whole loop can be expressed in Linq terms? e.g. along the lines of

l.UnionWith(conf.GetBoundList().SelectMany(kvp => { string key = kvp.Key.GetFullName(); return kvp.Value.Select(item => { if (item is string val) return new ConfigurationEntry(key, val); if (item is INode node) return new ConfigurationEntry(key, node.GetFullName()); throw new TangApplicationException($"Unable to serialize list of type {item.GetType()}"); }); }));

motus · 2019-01-08T01:02:09Z

lang/cs/Org.Apache.REEF.Tang/Formats/ConfigurationFile.cs

+                    {
+                        val = GetFullName((string)item);
+                    }
+                    else if (kvp.Value is INode)


shouldn't it be item is INode instead of kvp.Value is INode? also, can use if (item is INode node) ... syntax to avoid the cast

motus · 2019-01-08T01:05:53Z

lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationBuilderImpl.cs

+        public void BindList(INamedParameterNode iface, string impl)
+        {
+            IList<object> l;
+            if (!BoundLists.TryGetValue(iface, out l))


can join these two lines, i.e.

if (!BoundLists.TryGetValue(iface, out IList<object> l))

etc.

motus · 2019-01-08T01:06:04Z

lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationBuilderImpl.cs

+                l = new List<object>();
+                BoundLists.Add(iface, l);
+            }
+            l.Add((object)impl);


no need to cast

motus · 2019-01-08T01:07:28Z

lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationBuilderImpl.cs

+                {
+                    throw new ArgumentException("List cannot contain string that are null or empty");
+                }
+
                l.Add((object)n);


cast is redundant

Also, maybe we can write something like

if (impl.Any(string.IsNullOrEmpty)) { throw new ArgumentException("List cannot contain string that are null or empty"); } BoundLists.Add(iface, impl.ToList<object>());

markusweimer suggested changes Oct 9, 2018

View reviewed changes

singlis added 3 commits October 15, 2018 18:14

Merge remote-tracking branch 'origin/master' into REEF-2044

0aaf08a

Merge remote-tracking branch 'origin/master' into REEF-2044

354a880

- Added more unit tests, plus checks for null/empty strings in lists

f847c75

markusweimer approved these changes Oct 19, 2018

View reviewed changes

markusweimer suggested changes Oct 19, 2018

View reviewed changes

- Updating to tests to handle the binding exception for null and emp…

11b134d

…ty lists

jwang98052 reviewed Oct 19, 2018

View reviewed changes

interesaaat mentioned this pull request Jan 5, 2019

[REEF-2054] Elastic group communication: broadcast #1487

Open

motus self-requested a review January 5, 2019 00:46

motus suggested changes Jan 5, 2019

View reviewed changes

motus suggested changes Jan 8, 2019

View reviewed changes

		@@ -39,6 +39,7 @@ public interface IConfiguration
		ICollection<IClassNode> GetLegacyConstructors();

		IEnumerator<KeyValuePair<INamedParameterNode, object>> GetBoundSets();

		@@ -448,6 +587,7 @@ public void TestSetConfig()
		Assert.True(actual.Contains("six"));
		}

[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

Are you sure you want to change the base?

[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

Conversation

singlis commented Sep 12, 2018

singlis commented Sep 19, 2018 • edited Loading

singlis commented Oct 3, 2018 • edited Loading

markusweimer Oct 9, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusweimer Oct 9, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusweimer Oct 16, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

markusweimer Oct 9, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

markusweimer Oct 9, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

markusweimer Oct 9, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusweimer Oct 9, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

singlis commented Oct 19, 2018 • edited Loading

markusweimer left a comment

Choose a reason for hiding this comment

markusweimer commented Oct 19, 2018

markusweimer left a comment

Choose a reason for hiding this comment

jwang98052 commented Oct 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motus Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

motus Jan 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

motus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singlis commented Sep 19, 2018 •

edited

Loading

singlis commented Oct 3, 2018 •

edited

Loading

markusweimer Oct 9, 2018 •

edited by singlis

Loading

markusweimer Oct 9, 2018 •

edited by singlis

Loading

markusweimer Oct 16, 2018 •

edited by singlis

Loading

markusweimer Oct 9, 2018 •

edited by singlis

Loading

markusweimer Oct 9, 2018 •

edited by singlis

Loading

markusweimer Oct 9, 2018 •

edited by singlis

Loading

markusweimer Oct 9, 2018 •

edited by singlis

Loading

singlis commented Oct 19, 2018 •

edited

Loading

motus Jan 5, 2019 •

edited

Loading

motus Jan 5, 2019 •

edited

Loading