AMPL solver duplicate funcadd fix

[eslickj]

Fixes None

Summary/Motivation:

IDAES recently made a change where they set AMPLFUNC for external functions in IDAES on import. I'm not exactly sure of the details of why this was done, but it causes external functions to be imported twice. This is a minor change to only add lines from PYOMO_AMPLFUNC to AMPLFUNC if they are not already in AMPLFUNC.

Changes proposed in this PR:

• Check the AMPLFUNC environment variable contents before adding PYOMO_AMPL contents to avoid duplicate lines.

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[Robbybp]

This was done to work around this issue in ASL: ampl/asl#13. Thanks for this patch.

[jsiirola]

It is unfortunate that Black was allowed to muck with all the string endings (we generally use black -C -S for the codebase). One minor implementation comment and we can merge.

[Robbybp]

Can you make this change in pyomo.contrib.pynumero.interfaces.pyomo_nlp as well? The following patch should work:

diff --git a/pyomo/contrib/pynumero/interfaces/pyomo_nlp.py b/pyomo/contrib/pynumero/interfaces/pyomo_nlp.py
index 51edd0931..ce148f50e 100644
--- a/pyomo/contrib/pynumero/interfaces/pyomo_nlp.py
+++ b/pyomo/contrib/pynumero/interfaces/pyomo_nlp.py
@@ -92,15 +92,13 @@ class PyomoNLP(AslNLP):
             # The NL writer advertises the external function libraries
             # through the PYOMO_AMPLFUNC environment variable; merge it
             # with any preexisting AMPLFUNC definitions
-            amplfunc = "\n".join(
-                filter(
-                    None,
-                    (
-                        os.environ.get('AMPLFUNC', None),
-                        os.environ.get('PYOMO_AMPLFUNC', None),
-                    ),
-                )
-            )
+            amplfunc_lines = os.environ.get("AMPLFUNC", "").split("\n")
+            existing = set(amplfunc_lines)
+            for line in os.environ.get("PYOMO_AMPLFUNC", "").split("\n"):
+                # Skip (a) empty lines and (b) lines we already have
+                if line != "" and line not in existing:
+                    amplfunc_lines.append(line)
+            amplfunc = "\n".join(amplfunc_lines)
             with CtypesEnviron(AMPLFUNC=amplfunc):
                 super(PyomoNLP, self).__init__(nl_file)

I would just push it to this branch, but don't think I have permissions.

[eslickj]

@Robbybp, yes, I'll do it.

[Robbybp]

Looks good, thanks for the patch

[codecov]

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 88.38%. Comparing base (f697791) to head (befe271).

Files	Patch %	Lines
pyomo/solvers/plugins/solvers/ASL.py	0.00%	4 Missing ⚠️
pyomo/solvers/plugins/solvers/IPOPT.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3206      +/-   ##
==========================================
- Coverage   88.39%   88.38%   -0.01%     
==========================================
  Files         846      846              
  Lines       95153    95164      +11     
==========================================
+ Hits        84106    84113       +7     
- Misses      11047    11051       +4     

Flag	Coverage Δ
linux	86.33% <35.71%> (-0.01%)	⬇️
osx	76.16% <35.71%> (-0.01%)	⬇️
other	86.52% <57.14%> (-0.01%)	⬇️
win	83.82% <57.14%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jsiirola]

This looks good, and I won't hold up merging, but can we add a regression test to verify the expected behavior? Maybe in the future we can abstract this to a helper function so we don't have the same code in 3 locations.

[eslickj]

This looks good, and I won't hold up merging, but can we add a regression test to verify the expected behavior? Maybe in the future we can abstract this to a helper function so we don't have the same code in 3 locations.

@jsiirola, I can do both of these things in the next couple days. I'm not in a really big hurry for this to be merged, unless someone else is.