Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable slim on windows #1571

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Conversation

petrelharp
Copy link
Contributor

@petrelharp petrelharp commented Jul 5, 2024

Hopefully it is this simple! Edit: it was not.

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.84%. Comparing base (a011bf1) to head (15779b8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1571   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files         131      131           
  Lines        4431     4442   +11     
  Branches      608      607    -1     
=======================================
+ Hits         4424     4435   +11     
  Misses          3        3           
  Partials        4        4           

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

@petrelharp
Copy link
Contributor Author

Hm - getting errors like


stdpopsim\slim_engine.py:1665: in simulate
    self._run_slim(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = _SLiMEngine()
script_file = 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmppghxj46m.slim'
slim_path = 'slim', seed = None, dry_run = True, verbosity = None

    def _run_slim(
        self, script_file, slim_path=None, seed=None, dry_run=False, verbosity=None
    ):
        """
        Run SLiM.
    
        We capture the output using Popen's line-oriented text buffering
        (bufsize=1, universal_newlines=True) and redirect all messages to
        Python's logging module.
        By convention, messages from SLiM prefixed with "ERROR: " or
        "WARNING: " are treated as ERROR or WARN loglevels respectively.
        All other output on stdout is given the DEBUG loglevel.
        ERROR messages will raise a SLiMException here too, because
        they are always generated by the `stop()` eidos function which
        makes SLiM exit with a non-zero return code.
        """
        if slim_path is None:
            slim_path = self.slim_path()
    
        # SLiM v3.6 sends `stop()` output to stderr, which we rely upon.
        self._assert_min_version("4.0", slim_path)
    
        slim_cmd = [slim_path]
        if seed is not None:
            slim_cmd.extend(["-s", f"{seed}"])
        if dry_run:
            slim_cmd.extend(["-d", "dry_run=T"])
        if verbosity is not None:
            slim_cmd.extend(["-d", f"verbosity={verbosity}"])
        slim_cmd.append(script_file)
    
        with subprocess.Popen(
            slim_cmd,
            bufsize=1,
            universal_newlines=True,
            stdout=subprocess.PIPE,
            stderr=subprocess.PIPE,
        ) as proc:
            for line in proc.stdout:
                line = line.rstrip()
                if line.startswith("WARNING: "):
                    warnings.warn(
                        stdpopsim.UnspecifiedSLiMWarning(line[len("WARNING: ") :])
                    )
                else:
                    # filter `dbg` function calls that generate output
                    line = line.replace("dbg(self.source); ", "")
                    logger.debug(line)
    
            stderr = proc.stderr.read()
            for line in stderr.splitlines():
                if line.startswith("ERROR: "):
                    logger.error(line[len("ERROR: ") :])
    
        if proc.returncode != 0:
>           raise SLiMException(
                f"{slim_path} exited with code {proc.returncode}.\n{stderr}"
            )
E           stdpopsim.slim_engine.SLiMException: slim exited with code 1.
E           
E           ERROR (main): could not open input file: C:\Users\RUNNER~1\AppData\Local\Temp\tmppghxj46m.slim.

@petrelharp
Copy link
Contributor Author

petrelharp commented Jul 6, 2024

Uh-oh, and errors like this, which looks like a SLiM bug:

Error on script line 584, character 36:

    defineConstant("log", community.createLogFile("C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\test_sweep_meets_min_freq_at_s0\\sweep_start_af.logfile", logInterval=NULL));
                                    ^^^^^^^^^^^^^
FAILED tests/test_slim_engine.py::TestSelectiveSweep::test_sweep_meets_min_freq_at_end - stdpopsim.slim_engine.SLiMException: slim exited with code 1.
#ERROR (Eidos_WriteToFile): could not write to file at path D:\a\stdpopsim\stdpopsim/C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\test_sweep_meets_min_freq_at_e0\sweep_end_af.logfile.

@petrelharp
Copy link
Contributor Author

Okay, there are Issues with temporary files on Windows. In the tests there's various ways to deal with temporary files, but it's unclear what's going on because sometimes the github logs don't show us all the output (e.g. "slim exited with code 3" but not showing us the error).

To debug I've got a minimal but not-minimal-enough test set-up in #1574.

Also for some reason conda cacheing seems to not be working.

@petrelharp
Copy link
Contributor Author

Note: to make sure you see the full output from the tests (since the "Details" link tends to cut it off):

  1. scroll to the bottom, making sure you see the summary like ================== 27 failed, 9 passed, 1 warning in 12.00s =================== so you know For Reals you're at the bottom
  2. select a bit
  3. scroll to the top, shift-click to select the whole thing
  4. slowly scroll back through to the bottom so it all appears on the screen
  5. copy-paste to elsewhere

@petrelharp
Copy link
Contributor Author

Another note: we have in stdpopsim/slim_engine.py the _escape_eidos() function, that doubles up the backslashes in paths. This is necessary since otherwise we get errors like:

ERROR (EidosScript::Tokenize): illegal escape \U in string literal "C:".

Error on script line 21, character 36:

    defineConstant("trees_file", "C:\Users\RUNNER~1\AppData\Local\Temp\stdpopsim_bqckmhjx\8bcb71.trees");

@petrelharp
Copy link
Contributor Author

The logfile issue looks to be a SLiM bug: MesserLab/SLiM#452 maybe

@petrelharp
Copy link
Contributor Author

NotADirectoryError: [WinError 267] The directory name is invalid: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\stdpopsim_672p1gzn\\6c1381.slim

There seems to be some kind of issue with the temporary file code? (Note: this code was already changed from previously to deal with Windows not really doing temporary files in a reasonable or consistent way...)

@petrelharp
Copy link
Contributor Author

petrelharp commented Dec 23, 2024

Okay, one problem here is of the form

FAILED tests/test_slim_engine.py::TestCLI::test_simulate - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.

This happens when we do treeSeqOutput; however, it's not a problem with paths or permissions, since I can insert SLiM code immediately before this that writes to, reads from, and deletes the file in question and nothing bad happens.

Converting that code to hex we get 0xc0000409, which could mean a number of things; for instance, a buffer overrun or an unhandled exception. I think that converting it to hex is the thing to do here?
(Note: googling "3221226505" suggests that it's 0xc000007b; however (I think) that it is not, that seems to be google fucking up.)

@petrelharp
Copy link
Contributor Author

Success, sort of. I have discovered in #1638 that by removing the é from the citation for the relevant demographic model then the error that was plaguing me there dissappeared. However, this is very strange, since the symptom was that SLiM was crashing with exit code either 127 (according to echo $?), 3221226505 (according to print(proc.returncode)) or 0xc0000409 (converted to hex as above) *when calling treeSeqOutput. Not when reading the script.

@petrelharp
Copy link
Contributor Author

petrelharp commented Jan 5, 2025

Here's the tests that were not passing as of 9b8209:

FAILED tests/test_slim_engine.py::TestAPI::test_recap_and_rescale_on_external_slim_run - re.error: bad escape \U at position 32
FAILED tests/test_slim_engine.py::TestCLI::test_simulate - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_dfe_no_demography - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_dfe_no_interval - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_dfe_interval - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_dfe_annotation - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_dfe_bed_file - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_chromosomal_segment - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_chromosomal_segment_with_dfe_interval - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_chromosomal_segment_with_dfe_bed_file - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_chromosomal_segment_with_dfe_annotation - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestCLI::test_dry_run - stdpopsim.slim_engine.SLiMException: slim exited with code 3221226505.
FAILED tests/test_slim_engine.py::TestSelectiveSweep::test_global_sweep - UnicodeEncodeError: 'charmap' codec can't encode characters in position 22-23: character maps to <undefined>
FAILED tests/test_slim_engine.py::TestSelectiveSweep::test_local_sweep - UnicodeEncodeError: 'charmap' codec can't encode characters in position 22-23: character maps to <undefined>
FAILED tests/test_slim_engine.py::TestSelectiveSweep::test_sweeps_at_multiple_sites - UnicodeEncodeError: 'charmap' codec can't encode characters in position 22-23: character maps to <undefined>
FAILED tests/test_slim_engine.py::TestSelectiveSweep::test_sweep_with_background_selection - UnicodeEncodeError: 'charmap' codec can't encode characters in position 22-23: character maps to <undefined>
FAILED tests/test_slim_engine.py::TestCLI::test_dfe_demography - PermissionError: [Errno 13] Permission denied: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpk32dev6n'

And, changing the é to e in the citation above (in 07e9128) removed these two failures:

test_chromosomal_segment_with_dfe_bed_file
test_dry_run

and also the PermissionError went away - likely it is stochastic.

I suspect the problem comes because SLiM is writing the script itself into metadata.

@petrelharp
Copy link
Contributor Author

petrelharp commented Jan 5, 2025

Turns out open("file", "w") on windows defaults not to utf-8 but to something else that doesn't support unicode; appending encoding="utf-8" to the open( ) call for the script file fixes the problem. See MesserLab/SLiM#488

That fixed everything except test_recap_and_rescale_on_external_slim_run and the stochastic Permission errors.

@petrelharp
Copy link
Contributor Author

Holy crap that was NOT straightforward - see e.g., #1638 and previous takes.

But, this is Ready for Review!

escape eidos

utf-8 script file

re fix

maybe better temp file handling?

dry run fix?

even more escapes

changelog
Copy link
Collaborator

@nspope nspope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants