Add additional python path for use with EP-Launch

[matthew-larson]

Pull request overview

• Fixes PythonPlugin:SearchPaths Add Input File Directory to Search Path input not working with EP-Launch #8981
• This pull request adds epin to the python search plan. I utilized the epin path but had to use it's parent directory to get it to work. Also, state.dataSysVars->envinputpath1 is blank it this point in the code, which is why I used the get_environment_variable function to get the path. Tested in Windows using EP-Launch.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If any diffs are expected, author must demonstrate they are justified using plots and descriptions
• If changes fix a defect, the fix should be demonstrated in plots and descriptions

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts

[Myoldmopar]

Looking at this, I'm not sure this is the best path forward. I would kinda like EPLaunch itself to handle this custom behavior rather than E+ itself. We only add a few paths that we absolutely need to make the plugin system function and then allow the user to add custom folders for their workflows. While EP-Launch is our own tool and not some random thing, it's also an old tool that we are trying to replace with EPLaunch 3. I guess maybe this could've been an IDD switch. I dunno. @JasonGlazer what do you think?

[JasonGlazer]

I'm not against it but I think we should have a field in PythonPlugin:SearchPaths to turn this on or off.

[bonnema]

Could this be accomplished via the Add Current Working Directory to Search Path field? Not sure what EP-Launch uses for the working directory, but it could be set to the IDF directory before it is copied, then if the python file is alongside the original IDF it would be found.

[nealkruis]

@bonnema EP-Launch creates a new directory below the IDF that it copies the IDF into uses for the CWD. EP-Launch uses the epin environment variable to identify the original IDF directory, and that directory is added to the search path for other files referenced in the IDF (e.g., schedule files, WINDOW 5 data files).

I am okay adding a separate field as @JasonGlazer suggested, but that would mean it would have to wait for a future release since we are are wrapping up IO freeze for this release.

[bonnema]

I see. So the working and IDF directory are the same, and you cannot really switch either or else it will break other stuff.

[mjwitte]

Here's a suggestion. In epl-run.bat, when EnergyPlus is executed, use command line arguments to point to the input file, let the outputs default into the temporary CWD and everything else in epl-run.bat should finish up the same as before. There would need to be some logic to track whether the original idf, or epmif, or expidf should be executed (but that extension path could be set earlier as needed). That's assuming command line will accept those extensions. One might be tempted to use command-line to run ep-macro and expand objects, but I'm not sure command line supports the basement and slab preprocessors. You might need to use the "-s L" option also so the output file names are the same as elp-run.bat is expecting at the end. That's the default for suffixes.

[matthew-larson]

Hey @Myoldmopar @JasonGlazer @mjwitte, is the consensus that we should just hold off on this until the next release?

[Myoldmopar]

What if EPLaunch just copied any Python files into the run directory along with the IDF? It could be really intelligent about it and look for PythonPlugin:Instances and then only copy Python filenames from that object, or it be simpler and could copy Python files that match the IDF name or all the Python files or something. That would match what it is doing with the IDF itself, which seems like a good option and doesn't require breaking EnergyPlus operation.

[nealkruis]

I guess I don't see how the solution in this PR is breaking anything. EnergyPlus already looks for other files in the epin directory, and a user would likely expect the same for python modules. It might be nice to have an input switch, but that might also be confusing since there aren't equivalent input switches to search epin for other external file types.

I don't really see this as an EPLaunch issue. I think it should work for anyone setting epin.

[Myoldmopar]

Well, currently this PR is breaking things because it is trying to call back() on a blank fs::path which is causing a segfault in the debug builds. But obviously that could be fixed.

My main concern is introducing ambiguity into the Python search path. Right now the only path that is always searched is the EnergyPlus install directory. This has to be searched because that is where the pyenergyplus package lives. Other than that the user has full control over the Python search by using the SearchPaths object to flip on and off the different directories.

If we add this change in, then is it possible that the user will have a Python file in two different folders and it will try to find the wrong one and the user won't have control to avoid it? If we make EPLaunch carry over the files so that the IDF, EPW, and Python files stay together during the run, or better yet if we make EPLaunch use the command line arguments so that it doesn't have to move anything around, then we don't have to introduce anything like this into EnergyPlus.

While I get that EnergyPlus searches for things in CheckForActualFilePath, I really think that by encouraging better use of the command line arguments, we can push things toward a better place. Right now I am unaware of any testing we do on the epin behavior until someone just sits down with EPLaunch and tries to run an idf with an external Schedule:File or something. Whereas we test the command line argument workflow every single commit. If this change goes in, it needs to be accompanied by testing changes that exercise epin, including files with different files named the same but in two folders to ensure that EnergyPlus is picking up the right version of the file.

[nealkruis]

That's fair @Myoldmopar. My feeling is that more users are going to be tripped up by not searching epin when it is set by EPLaunch than accidentally having identically named files in epin and another search path. If an EPLaunch user has a module in epin it's likely because they intend for it to be found. If a non-EPLaunch user has a module in epin, they are likely consciously setting epin for the purpose of finding files.

Maybe its worth re-thinking whether we want separate behavior for python modules and other external files. I think there is some utility in users being able to set a search path outside of EXE / IDF through an environment variable (e.g., if multiple users have a shared repo of python modules and they don't want to change the paths in the IDFs every time they run them on a different machine).

I agree that if this goes in, it should be explicitly tested in integration tests.

[nrel-bot]

@matthew-larson @lgentile it has been 28 days since this pull request was last updated.

[Myoldmopar]

Now that we are now in flexible I/O time, I think it would be good to wrap this one up. Earlier in the conversation I think we are felt OK adding a new input switch to solve this issue, but we were past I/O freeze. If we still feel that way, I'm happy to pick up this branch and modify it so it is protected by a new input field.

[nrel-bot]

@matthew-larson @lgentile it has been 28 days since this pull request was last updated.

[nrel-bot-3]

@matthew-larson @lgentile it has been 28 days since this pull request was last updated.

[Myoldmopar]

How do we feel about adding a new field to the SearchPaths object for searching the epin environment variable, with a default of YES? I'll do that directly if that's a reasonable solution. I would personally prefer if it were turned off by default, but I'm willing to concede that to make it work more seamless for more users.

New field:

   A4, \field Use epin Environment Variable for Search Path
       \note The "epin" environment variable is set by some EnergyPlus interfaces
       \note If this is enabled, and that variable is set, the value of the variable
       \note will be added to the Python search path.
       \required-field
       \type choice
       \key Yes
       \key No
       \default Yes

Original A4 and beyond will be moved down one index with a transition rule. Documentation will be updated accordingly.

Let me know if that works or doesn't work and I'll do it and fix the seg fault as well.

[Myoldmopar]

Alright, I think I got all the pieces in place. I tested the new input field and was able to find files based on epin if the field was set. I tested the simple transition as well and it worked fine. No input files include the search paths object right now, so no files required changes for now. I'll walk through my updates now but I suspect this is ready unless someone knows of something else to add.

[Myoldmopar]

I'm good with these changes, but I'll let CI run on it before merging. If anyone sees anything missing, or you want to add anything @nealkruis or @matthew-larson have at it or let me know.

[Myoldmopar]

Small description of the new input field.

[Myoldmopar]

New input field to allow responding to the epin environment variable, defaulted to ON. Of course incremented all following input field numbers.

[Myoldmopar]

• Assume the input field value is true, then read the value from the IDF
• If it is true, then try to read the environment variable value
• If the environment variable doesn't exist, issue a warning for that odd combination
• Otherwise read the environment variable value, and if the path exists, add it to the Python search path.
  • I thought about this one for a bit. Should we concern ourselves with checking if the path exists on the drive? I mean maybe, maybe not, but in this case, the Python interpreter is literally milliseconds away from actually using that path to find Python stuff, so I leaned on providing a nice warning if the directory doesn't exist.

[Myoldmopar]

We don't need to throw fatal errors in here, just issue continuations and return, the code is already destined for a more meaningful fatal error at this point.

[Myoldmopar]

Simple transition rule to increment the arg count by one and push everything past by one spot.

[Myoldmopar]

Alright, Clang format caught something, but I've got that fixed locally. Otherwise, CI looks to be coming back clean. I'm going to push the Clang format fix and then merge this right in. Goodnight all.