Add support for Elixir #447

lucasavila00 · 2021-08-08T04:03:40Z

No description provided.

* Add support for Elixir * try to fix CI * try to fix CI * try to fix CI * try to fix CI * revert changes on CI file * revert fomatting changes * enable all tests * update readme * revert change to CI file * fix api test * fix init.py fmt

lucasavila00 · 2021-08-08T12:33:13Z

I see codecov is failing, I guess it's because I haven't added tests to the interpreter folder, only e2e.

I'll add the missing tests.

lucasavila00 · 2021-08-09T13:09:36Z

For some context on the decisions made in the Elixir Interpreter (ie: why isn't it returning a list? why does it return a binary?):

StrikerRUS

@lucasavila00 Thank you so so much! This is amazing! I gave this PR a brief look and can say it is high quality. Unfortunately, I'm not familiar with Elixir, so I need some time to learn the basics and read the links you've provided above in order to give a thoughtful review. Given that you've already written end-to-end tests for Elixir and they are passed, I believe there might be only minor requests for changes.
However, I've left some initial questions I can ask knowing nothing about Elixir below.

I see codecov is failing, ...

Please ignore this for now. I'll help to deal with all formal tests, linting errors, supporting code and so on.

Also, I'll be very grateful if you could write one or two sentences disclosing the story behind this PR: how did you find m2cgen and why Elixir 🙂 .

m2cgen/cli.py

m2cgen/interpreters/elixir/code_generator.py

m2cgen/interpreters/elixir/interpreter.py

tests/e2e/test_e2e.py

lucasavila00 · 2021-08-10T11:15:01Z

Thanks for the kind words: )

Also, I'll be very grateful if you could write one or two sentences disclosing the story behind this PR: how did you find m2cgen and why Elixir slightly_smiling_face .

I have been using m2cgen on my job for more than a year. We use it to convert models to JS, mostly. I'm a super fan of m2cgen. I had read the source code of m2cgen a bit, and I'm super comfortable analyzing the generated files.

I have been learning elixir for the past months, and I missed having m2cgen available for Elixir.

Elixir is a great language for hosting m2cgen compiled modules, as the language/vm have native support for loading new models while the old ones are executing, for instance. ( https://blog.appsignal.com/2018/10/16/elixir-alchemy-hot-code-reloading-in-elixir.html https://blog.appsignal.com/2021/07/27/a-guide-to-hot-code-reloading-in-elixir.html )

Also, the language/vm parallelism would work really well with m2cgen generated models, allowing it to scale basically for free.

StrikerRUS · 2021-09-03T20:23:04Z

I have been using m2cgen on my job for more than a year.

Wow, impressive! Thanks a lot for sharing your experience.

I'm sorry for not getting back to you earlier, but I was reading official Elixir guide to get a little bit familiar with the language and its opportunities. Now I'm done and will provide a thoughtful review for this PR very soon.

m2cgen/interpreters/elixir/code_generator.py

StrikerRUS · 2021-10-07T22:57:46Z

Sorry for the bustle here 😬 - just wanted to be sure this PR is playing well with recently updated major versions of the dependencies.

lucasavila00 · 2021-10-19T11:36:04Z

Sure, I'll do it all.
I'm sorry for taking this long, I have been on a rush lately but I haven't forgotten about this project.

StrikerRUS · 2022-01-13T15:19:43Z

Kindly ping @lucasavila00 🙂

lucasavila00 · 2022-01-14T13:26:46Z

@StrikerRUS I'm sorry. I plan to have it done this weekend. If I'm unable to do it I close the MR.

StrikerRUS · 2022-01-15T00:39:08Z

I plan to have it done this weekend.

Super!

If I'm unable to do it I close the MR.

Please don't do it! I strongly believe your contribution is extremely valuable. As you can see, there is no rush at all 😉 . If you can do it next weekend or even some time later, that's fine. Also, if you have no time at all to finish this PR, that's totally OK as well. I think I can take it over (push some changes directly into your fork or done them via a follow-up PRs) if you don't mind.

* Receive and return lists * fix lint issue * fix api tests

lucasavila00 · 2022-01-16T16:41:56Z

@StrikerRUS aside from the issue of NaN not being supported by Erlang VM, I think the code is ready.

But there's a flaky test and I'm not sure what to do (failed 1/5 times):

tests/e2e/test_e2e.py::test_e2e[xgboost_XGBClassifier - elixir - train_model_classification_binary2]

E           RuntimeError: Bad exit code (1), stderr:
E           ** (ArithmeticError) bad argument in arithmetic expression
E               (stdlib 3.15.2) :math.exp(143625999918830.8)
E               /tmp/tmp_swiqff8/score.ex:27: Model.sigmoid/1
E               /tmp/tmp_swiqff8/score.ex:24: Model.score/1
E               /tmp/tmp_swiqff8/score.ex:36: Runner.run/0
E               (stdlib 3.15.2) erl_eval.erl:685: :erl_eval.do_apply/6
E               (elixir 1.12.0) lib/code.ex:656: Code.eval_string_with_error_handling/3

In the end :math.exp(143625999918830.8) is trying to calculate a float that's bigger than erlang can represent: https://stackoverflow.com/a/38533723

lucasavila00 · 2022-01-16T16:44:50Z

m2cgen/interpreters/elixir/code_generator.py

@@ -43,6 +49,7 @@ def add_function_def(self, name, args, is_scalar_output):
        self.add_code_line(function_def)

        self.increase_indent()
+        self.add_code_line("input = list_to_binary(input)")


Is there a less hacky way to do this?

The name of the overridden method add_function_def suggests this is a generic function definition, not the "score" function.

Sorry for the late response - was busy these days. TBH, I don't want to delay this PR and bother you with any other investigations or significant code refactoring. I think this approach is OK for now. Later I can refactor this.

Only one thing bothers me,

But there's a flaky test and I'm not sure what to do (failed 1/5 times):

do you mean that right now that test can fail any time? It's quite odd because I believe all random seeds are fixed in tests... BTW, I remember we had another problem with this particular test: #205. If so, let's just skip that test for Elixir for now similarly to how tests with missing values (NaNs) are being skipped ring now. WDYT?

do you mean that right now that test can fail any time? It's quite odd because I believe all random seeds are fixed in tests...

yeah, it happened a few times randomly - my perception is that it happens 20% of the time

If so, let's just skip that test for Elixir for now similarly to how tests with missing values (NaNs) are being skipped ring now. WDYT?

I tried to do this, but I couldn't figure out how to skip a specific test in the test suite. Would you mind pointing me how to do it?

I think we can simply deselect that test for now here

m2cgen/.ci/test.sh

Line 17 in 333d808

pytest -v "-m=$LANG" tests/e2e/

via -k option:

pytest -v "-m=$LANG" "-k=not(xgboost_XGBClassifier and elixir and train_model_classification_binary2)" tests/e2e/

* return list directly * don't return scalar output as list

StrikerRUS · 2022-02-16T22:49:31Z

@lucasavila00 Thank you so much for this contribution again! 🚀

I'm going to merge this PR and after doing that make some small improvements in a follow-up PRs. If there would a need in high-level expertise in Elixir I'll @ ping you, if you don't mind 🙂 .

lucasavila00 · 2022-02-16T23:31:54Z

I'm glad this was merged. I'm sorry for missing the message on the -k option.

Add support for Elixir (#1)

6a67340

* Add support for Elixir * try to fix CI * try to fix CI * try to fix CI * try to fix CI * revert changes on CI file * revert fomatting changes * enable all tests * update readme * revert change to CI file * fix api test * fix init.py fmt

Add tests for Elixir Interpreter (#2)

7ea59f4

StrikerRUS reviewed Aug 10, 2021

View reviewed changes

m2cgen/cli.py Show resolved Hide resolved

m2cgen/interpreters/elixir/code_generator.py Show resolved Hide resolved

m2cgen/interpreters/elixir/interpreter.py Show resolved Hide resolved

tests/e2e/test_e2e.py Show resolved Hide resolved

StrikerRUS reviewed Sep 8, 2021

View reviewed changes

m2cgen/interpreters/elixir/code_generator.py Show resolved Hide resolved

Merge branch 'master' into master

6646e16

lucasavila00 added 2 commits January 16, 2022 11:16

Merge branch 'BayesWitnesses:master' into master

d677440

Receive and return lists (#3)

f27e4ca

* Receive and return lists * fix lint issue * fix api tests

lucasavila00 commented Jan 16, 2022

View reviewed changes

lucasavila00 and others added 2 commits January 16, 2022 14:41

Return list directly, improve generated code readability (#4)

4f41ce6

* return list directly * don't return scalar output as list

Merge branch 'master' into master

7fd6800

Merge branch 'master' into master

76d5f50

StrikerRUS approved these changes Feb 19, 2022

View reviewed changes

StrikerRUS merged commit 9c7e0f4 into BayesWitnesses:master Feb 19, 2022

StrikerRUS mentioned this pull request Feb 19, 2022

Skip flaky Elixir test #507

Merged

StrikerRUS mentioned this pull request Apr 30, 2022

Support for SAS - happy to open PR with some guidance #517

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Elixir #447

Add support for Elixir #447

lucasavila00 commented Aug 8, 2021

lucasavila00 commented Aug 8, 2021

lucasavila00 commented Aug 9, 2021

StrikerRUS left a comment

lucasavila00 commented Aug 10, 2021

StrikerRUS commented Sep 3, 2021

StrikerRUS commented Oct 7, 2021

lucasavila00 commented Oct 19, 2021

StrikerRUS commented Jan 13, 2022

lucasavila00 commented Jan 14, 2022

StrikerRUS commented Jan 15, 2022

lucasavila00 commented Jan 16, 2022

lucasavila00 Jan 16, 2022

StrikerRUS Jan 23, 2022

lucasavila00 Jan 27, 2022

StrikerRUS Jan 31, 2022

StrikerRUS commented Feb 16, 2022

lucasavila00 commented Feb 16, 2022

Add support for Elixir #447

Add support for Elixir #447

Conversation

lucasavila00 commented Aug 8, 2021

lucasavila00 commented Aug 8, 2021

lucasavila00 commented Aug 9, 2021

StrikerRUS left a comment

Choose a reason for hiding this comment

lucasavila00 commented Aug 10, 2021

StrikerRUS commented Sep 3, 2021

StrikerRUS commented Oct 7, 2021

lucasavila00 commented Oct 19, 2021

StrikerRUS commented Jan 13, 2022

lucasavila00 commented Jan 14, 2022

StrikerRUS commented Jan 15, 2022

lucasavila00 commented Jan 16, 2022

lucasavila00 Jan 16, 2022

Choose a reason for hiding this comment

StrikerRUS Jan 23, 2022

Choose a reason for hiding this comment

lucasavila00 Jan 27, 2022

Choose a reason for hiding this comment

StrikerRUS Jan 31, 2022

Choose a reason for hiding this comment

StrikerRUS commented Feb 16, 2022

lucasavila00 commented Feb 16, 2022