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 #1342

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

petrelharp
Copy link
Contributor

I'm having a start at #1305 here.

@codecov
Copy link

codecov bot commented Aug 28, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.08 ⚠️

Comparison is base (5bb5b71) 99.85% compared to head (be9ff1c) 99.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
- Coverage   99.85%   99.77%   -0.08%     
==========================================
  Files         117      117              
  Lines        4032     4037       +5     
  Branches      566      565       -1     
==========================================
+ Hits         4026     4028       +2     
- Misses          3        5       +2     
- Partials        3        4       +1     
Impacted Files Coverage Δ
stdpopsim/cli.py 99.10% <100.00%> (-0.68%) ⬇️
stdpopsim/slim_engine.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@petrelharp petrelharp force-pushed the windows_slim branch 2 times, most recently from 89c161b to ba2aac0 Compare August 29, 2022 05:45
@petrelharp
Copy link
Contributor Author

There seems to be some issues with TemporaryDirectory in python 3.7 on Windows: https://bugs.python.org/issue26660 ... so, perhaps we will not support that combination?

@petrelharp petrelharp changed the title enable tests (this will not work) enable SLiM on Windows Aug 29, 2022
@petrelharp
Copy link
Contributor Author

Hmph, I'm getting inscrutible

E           stdpopsim.slim_engine.SLiMException: slim exited with code 3.

errors. Maybe someone can have a go at this on an actual windows box?

@petrelharp
Copy link
Contributor Author

Looks like to get this totally working we'll need to fix MesserLab/SLiM#349 and build SLiM from source until a release happens.

@petrelharp
Copy link
Contributor Author

Ben has commited a fix to MesserLab/SLiM#349; so we can now either build SLiM from source to run these tests or (this is my vote) wait for a release to enable this testing.

@petrelharp
Copy link
Contributor Author

Tagging @andrewkern

@petrelharp
Copy link
Contributor Author

However, @bhaller says it'd be nice to test SLiM on windows this way, so perhaps we should get this going?

@bhaller
Copy link

bhaller commented Sep 8, 2022

Mostly it'd be good to make sure no further Windows problems will bite stdpopsim after SLiM 4.0.1 rolls. It's infinitely easier to fix issues before rolling than to do a new roll just to fix an issue found post-roll. :-> I don't want to do a 4.0.2 unless the sky is falling. :->

@andrewkern
Copy link
Member

i think we can build slim on windows simply enough like we do over on pyslim in a GH action- would that work?

@grahamgower
Copy link
Member

Yeah, we used to build SLiM from sources in CI so this should be easy enough.

@bhaller
Copy link

bhaller commented Sep 14, 2022

i think we can build slim on windows simply enough like we do over on pyslim in a GH action- would that work?

SLiM builds itself on Windows in its GitHub Actions workflow, so you can look there if you hit any bumps in the road:

https://github.com/MesserLab/SLiM/blob/master/.github/workflows/tests.yml

@petrelharp
Copy link
Contributor Author

I've restarted the tests because SLiM 4.0.1 is released, so that should fix the logflie problem.

@bhaller
Copy link

bhaller commented Sep 14, 2022

Still failed; looks like slim 4.0 might be hard-coded somewhere, @petrelharp?

2022-09-14T19:23:30.6130695Z   added / updated specs:
2022-09-14T19:23:30.6131434Z     - msprime==1.2.0
2022-09-14T19:23:30.6131904Z     - slim==4.0

4.0.1 is available on conda and should fix this problem.

@nspope
Copy link
Collaborator

nspope commented Sep 22, 2022

Once #1343 is in this should be fixed ?

@petrelharp
Copy link
Contributor Author

Hopefully I rebased this correctly - need to double-check some of those tests that maybe got removed.

use tempdir, closes popsim-consortium#427

.

temp file names

better context handling

extra level of escaping in eidos

escape logfile

cache bump

.
@petrelharp
Copy link
Contributor Author

Well, darn, I was hoping this was All Fixed, but it appears to not be fixed yet. I can't jump on this just now, but if anyone else is itching to do it, go for it (or remind me in a few weeks?)

@petrelharp
Copy link
Contributor Author

For future reference, we're getting errors on Windows like:

_____________ TestAPI.test_recap_and_rescale_on_external_slim_run _____________
...
         out = re.sub(
            'defineConstant\\("trees_file.+;',
            f'defineConstant("trees_file", "{treefile}");',
>           out,
        )

tests\test_slim_engine.py:293: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
C:\Miniconda\envs\stdpopsim\lib\re.py:194: in sub
    return _compile(pattern, flags).sub(repl, string, count)
C:\Miniconda\envs\stdpopsim\lib\re.py:311: in _subx
    template = _compile_repl(template, pattern)
C:\Miniconda\envs\stdpopsim\lib\re.py:302: in _compile_repl
    return sre_parse.parse_template(repl, pattern)
...                    try:
                        this = chr(ESCAPES[this][1])
                    except KeyError:
                        if c in ASCIILETTERS:
>                           raise s.error('bad escape %s' % this, len(this))
E                           re.error: bad escape \U at position 32

C:\Miniconda\envs\stdpopsim\lib\sre_parse.py:1018: error

... but also


        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 3.

stdpopsim\slim_engine.py:1685: SLiMException
------------------------------ Captured log call ------------------------------
WARNING  stdpopsim.cli:cli.py:931 Simulation information:
    Engine: slim (4.0.1)
    Model id: PiecewiseConstant
    Model description: Piecewise constant size population model over multiple epochs.
    Seed: 1234
    Population: number_samples (sampling_time_generations):
        pop_0: 5 (None)
Contig Description:
    Contig origin: chr1:0-248956
    Contig length: 248956.0
    Contig ploidy: 2
    Mean recombination rate: 1.1523470111585671e-08
    Mean mutation rate: 1.29e-08
    Genetic map: None

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.

5 participants