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

Add Windows support. #31

Open
thomaskeller79 opened this issue Jul 29, 2016 · 30 comments
Open

Add Windows support. #31

thomaskeller79 opened this issue Jul 29, 2016 · 30 comments

Comments

@thomaskeller79
Copy link
Member

Original report by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Florent Teichteil-Koenigsbuch recently provided a patch (which is attached) that allows to run the planner on Windows. We should evaluate the planners performance on Windows before we incorporate the (awesome, thanks Florent!) addition into the main repo.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Once this is included, we should make sure that all future changes still compile and run on Windows. As not all developers have access to a Windows machine, we have to find a way how to make this automatically. We have a Windows machine running here in Basel where the FastDownward developers perform compilation and performance tests as well, and it should be possible to create a script that compiles PROST automatically on that machine and sends an email if compilation fails. We can also use that machine to run some instances and see if the output is as expected. Note that, even though it would be nice to also have performance tests, we have no access to a Windows grid such that this is not a possibility.

If you agree on this, we therefore need the following scripts (as part of this issue):

  1. A script that checks out PROST and compiles it on the Windows machine here in Basel whenever code is pushed to the prost (or better prost-dev?) repository.
  2. A second script that runs some (small) instances and checks if the output is as desired.

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


There is a problem with fdd.h library included in search_engine.h in line 16. There is no patch instructions for handling that and I cannot find windows version of this lib. The library can be found at BuDDy webpage. Is it an option to download the lib and include it directly if target OS is Windows?

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


There is some discussion on this topic on the archive of the buddy-users mailing list which can be found here: https://sourceforge.net/p/buddy/mailman/buddy-users/. It seems that it is possible to compile the library if the right compiler is used, so I see two possibilities how to proceed (and neither includes the library into our code base):

  1. find out how to compile libbdd on Windows and describe the process on the Windows installation wiki page
  2. remove all BDD functionality from the Windows version - anyway, there isn't much left and I don't know how important it is

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


I found a way to install BuDDy to windows after a short discussion and the answer was to install it from a website of a developer that works on porting Linux libraries to Cygwin via cygport. This might be a good solution as long as this guy keeps his website up with all the libs.

As the second proposition for solving the problem concerning BuDDy (removing BuDDy functionality from Windows version), these are the variables and methods that are being used:

#!c++

// BDD related methods
bdd stateToBDD(KleeneState const& state) const;
bdd stateToBDD(State const& state) const;
bool BDDIncludes(bdd BDD, KleeneState const& state) const;

// The BDDs where dead ends and goals are cached
static bdd cachedDeadEnds;
static bdd cachedGoals;

So, in case that solution is better, their importance should be discussed.

However, before everything, we should decide on which ways should this port to windows be implemented (one or multiple). Possible ways to do it are using:

Cygwin and MinGW don't work good together, it's possible but their communication is complicated and relying to this combination is a bad thing in my opinion. I started porting the app with Cygwin (and succeeded to install BuDDy with it) but current patch notes suggest using flag -mwin32 (which also doesn't cover 64 bit OS case, if that is even needed?) hence the compiler is trying to access MinGW compiler. I did not find a way to install BuDDy with MinGW and buddy-user mailing lists do not give any answers and are obsolete.

So, in my opinion, it would be good to chose the way how porting to windows should be done and if multiple ways are to be done, in which order and priority, so that we would have a version for windows soon. It would also be good to contact Mr. Teichteil and discuss his initial idea and work like that.

One more note, which different versions of Windows should be covered? Cygwin stopped support for Windows versions older than Windows 7 and from Windows 10 on, there is Bash on Windows that enables users to run native Linux apps on windows without any additional setup (I already tested it and it works like a charm :)).

Sorry for a long comment, I hope that it is at least clear as it is long.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


However, before everything, we should decide on which ways should this port to windows be implemented (one or multiple).

If we leave the external dependencies out of the discussion for now (BuDDy can be removed for Windows easily), I think we should aim for a true Windows version, which is, in my understanding, running the code from Visual Studio. As far as I understand it, Cygwin emulates a Linux-like environment, and I think that the code should run there already.

It would also be good to contact Mr. Teichteil and discuss his initial idea and work like that.

I'll point Florent to this discussion

One more note, which different versions of Windows should be covered?

I honestly have no idea about Windows version. Just start with the one you have, and then we'll see if it also runs on other (newer) version.

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


@thomaskeller79 Is there a particular reason why State is defined as struct in states.h file in rddl_parser and like a class in search? I'm experiencing issues with linking and if I change it to class and make all methods public, some of the linking issues go away.

Also, the patch doesn't cut it for all the changes that need to be done. One of the issues is with timer.h class and the methods used there. Are those modifications done (and compatible in parser and search)? If yes, I can proceed and adapt them to Windows, if not, maybe that should be done firstly.

For now, I'm generating VS project successfully but am still setting proper flags for correct compilation of the project.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Is there a particular reason why State is defined as struct in states.h file in rddl_parser and like a class in search? I'm experiencing issues with linking and if I change it to class and make all methods public, some of the linking issues go away.

No, there is no reason for that, it's historical and will be changed with issue #33 anyway, so go ahead and change it accordingly.

One of the issues is with timer.h class and the methods used there. Are those modifications done (and compatible in parser and search)?

Which modifications do you mean? Currently, no one is working on issue #33, so you can go ahead and adapt the search and parser code.

Also, the patch doesn't cut it for all the changes that need to be done. [...] If yes, I can proceed and adapt them to Windows, if not, maybe that should be done firstly.

I feared so. Please go on and make sure that everything that is crucial (and the timer is crucial for the learning part of the IDS heuristic) runs on Windows.

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


@thomaskeller79 Building and compilation in CMake on Windows now works. Haven't tested anything yet, but I noticed that current build.py script, while it compiles the project, doesn't create .sln file that represents Visual Studio solution file (which when double-clicked opens VS project). CMake can create .sln, with plain running of cmake command so if that is still the idea, I can add that to build.py script. Note: Current build behavior is identical to the one of FastDownward.

The code is online. Testing is due to be done, checking if all corresponding flags are included and adding comments to CMakeLists files that explain Windows tags.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Building and compilation in CMake on Windows now works. Haven't tested anything yet, but I noticed that current build.py script, while it compiles the project, doesn't create .sln file that represents Visual Studio solution file (which when double-clicked opens VS project). CMake can create .sln, with plain running of cmake command so if that is still the idea, I can add that to build.py script. Note: Current build behavior is identical to the one of FastDownward.

Since I don't use VS, I do not have any preference. If you were to develop PROST in VS, would you like the solution file to be generated automatically? If so, add it. Otherwise, leave it.

The code is online. Testing is due to be done, checking if all corresponding flags are included and adding comments to CMakeLists files that explain Windows tags.

Sounds great, I'll try to have a look at the code as soon as possible. In the meanwhile, we should think about doing some performance tests. If we want to do this properly, we should run all the benchmarks on a single machine, even if it takes a couple of days. We could use our groups Windows PC for that.

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


Since I don't use VS, I do not have any preference. If you were to develop PROST in VS, would you like the solution file to be generated automatically? If so, add it. Otherwise, leave it.

Personally, I would never use Visual Studio for C++ development. Even now I did everything from Visual Studio Command Prompt and a random editor. There can be a short instruction (make a folder, change to that folder and run cmake PROST_LOCATION/src/) for generating .sln file and others needed for opening PROST in VS so we can just simply add this in the wiki.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


I tried to compile the planner on Windows (using some VS shell) and got the following error message:

C:\thomas-prost>python build.py
Building configuration release
-- The C compiler identification is MSVC 19.0.23026.0
-- The CXX compiler identification is MSVC 19.0.23026.0
-- Check for working C compiler: C:/Program Files (x86)/Mic
14.0/VC/bin/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Mic
14.0/VC/bin/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/M
o 14.0/VC/bin/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/M
o 14.0/VC/bin/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found BISON: C:/win_flex_bison-latest/bison.exe (found v
-- Found FLEX: C:/win_flex_bison-latest/flex.exe (found ver
CMake Warning at search/CMakeLists.txt:25 (message):
  BuDDy lib was not found.  Since this is a Windows build B
  included in the build.


CMake Error: The following variables are used in this project, but they are set
to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake file
s:
FL_LIBRARY (ADVANCED)
    linked by target "rddl-parser" in directory C:/thomas-prost/src/rddl_parser

-- Configuring incomplete, errors occurred!
See also "C:/thomas-prost/builds/release/CMakeFiles/CMakeOutput.log".
Built configuration release failed due to CalledProcessError

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


I forgot to write here the instructions. You have to install Flex from http://gnuwin32.sourceforge.net/packages/flex.htm and Bison from gnuwin32.sourceforge.net/packages/bison.htm and add C:\GnuWin32\bin and C:\GnuWin32\include to System Variables path. That fixes the issue you got.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


But why does it say "Found FLEX" and "Found BISON" then?

@thomaskeller79
Copy link
Member Author

Original comment by Relić Đorđe (Bitbucket: 557058:f6befa2c-c3fc-4e1b-965b-ab7eca65a967, ).


The distribution from which you installed Flex and Bison doesn't contain FL_LIBRARY while the when you install flex from the link I provided, you have the needed library and searching for it is covered with CMake.

@thomaskeller79
Copy link
Member Author

Original comment by osrunescape080417 (Bitbucket: 5b39a13ed38a522e77ff7640, ).


I have been studying AI Planning since the middle of April and PDDL since the middle of May.

I am intrigued by RDDL and PROST.

I would like to compile PROST on my Windows XP machine.

I have read issue #31 (i.e., this issue) and patch.txt.

Is there a way that I can get the source code for the work that has been done on issue #31 so far?

You don't have to go to a lot of trouble for me; if you want, just put all of the source code that requires changes into a single file and I will sort it all out.

To show my appreciation, if you desire, I can provide documentation so that others can use my recipe to compile it on their Windows machines.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Hi @osrunescape080417

we are happy to share our code with you, and if you manage to get things to run on Windows we'd be happy to include your changes into the repo. I grant you read access to the developer repository, there you can find what we have so far under the branch issue-31.

If you need any more help, please let us now!

Best,
Thomas

@thomaskeller79
Copy link
Member Author

Original comment by osrunescape080417 (Bitbucket: 5b39a13ed38a522e77ff7640, ).


Thank you, Doctor Keller.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


@osrunescape080417 I'm not sure how else to contact you - could you send me an email at some point to tho.keller@unibas.ch if you are still interested in this topic, so I can get in contact directly?,
Anyway, I wanted to let you know that we merged issue #38 a few days ago, which presumably affects this issue quite heavily. If you are still working on a Windows version of Prost, it would probably be good to merge the current default branch into your repo as soon as possible.

@thomaskeller79
Copy link
Member Author

Original comment by geisserf (Bitbucket: 557058:e7a9f9a5-3ea8-4154-97d2-10446425dce3, GitHub: geisserf).


I would like to start working on this issue when I have some time over the next week. The major problem with Windows support right now seems to be the BuDDy library. Since most of the links provided in the above discussions are dead nowadays I think the best way to deal with this is to remove BuDDy from the planner, at least for Windows support (although we should also evaluate how much performance BuDDy actually brings).

As far as I see it the only part where we use BDDs is in goal and dead end checks. It seems to me that an alternate approach would be just to introduce state caches for dead ends and goal states. This presents a problem if there will be exponentially many dead ends or goal states, but right now I don’t know of any domain where this actually happens. We could also think of using a more popular BDD library which has Windows support instead. What do you think @{557058:280236d3-4090-4dc9-9a03-b6e1425df4e7} ?

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


The number of dead ends or goals is exponentially in many cases. In crossing traffic, for instance, the goal is reached once the agent reaches the goal cell, but the position of the stones / obstacles is irrelevant.

Nonetheless, I agree that we should evaluate how important the usage of the goal and dead end detection is - I wanted to look into this for quite a long time. I started an experiment and will report on the results here once it has finished.

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Results for 2011/2014 domains with/without BuDDy usage. Turns out that the bdds only help in crossing traffic. Experiments for IPC 2018 domains are still running.

@thomaskeller79
Copy link
Member Author

Original comment by geisserf (Bitbucket: 557058:e7a9f9a5-3ea8-4154-97d2-10446425dce3, GitHub: geisserf).


For other domains, at least in the IPPC2014 configuration, the results are actually better without BuDDy usage. Do you know if this is a statistical artefact or is this really a result from having less overhead during search?

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


The largest differences for the IPC 2014 configuration are in triangle tireworld, crossing traffic and navigation, and these are indeed domains where reward locks were detected during training. The small differences of up to \pm 0.02 in elevators, game of life, traffic and skill teaching must be statistical noise because no reward locks were detected during the training phase and bdd usage is hence turned off for both planner configuration. Finally, the difference for wildfire might be explained by BDD usage (there are 3 instances where reward locks are detected), but it could also be noise.

The IPC 2011 configuration always had a larger variance, as can be seen in sysadmin: there, both planners are identical (no reward locks detected in sysadmin), but the difference is 0.07!

@thomaskeller79
Copy link
Member Author

Original comment by tkeller (Bitbucket: 557058:280236d3-4090-4dc9-9a03-b6e1425df4e7, GitHub: thomaskeller79).


Providing results for IPC 2018 is harder than expected, because the scripts do not work for those domains. I’ll see if I can dedicate time to this today or not.

@thomaskeller79
Copy link
Member Author

Original comment by geisserf (Bitbucket: 557058:e7a9f9a5-3ea8-4154-97d2-10446425dce3, GitHub: geisserf).


I see, so BDDs are never detrimental. I think we could also measure the difference to using a normal map to cache states. It’s true that in some cases there might be exponentially many goal states, but adding the states to the BDD (by applying the or operator) also induces some overhead, so it might be interesting to see how a naive caching approach performs (but maybe you already tried this back when you implemented the BDD approach?). Especially since BDDs for goal formula can also become exponentially large (e.g. when encoding the goal description of Connect Four).

@geisserf
Copy link
Contributor

I started to work on this issue today and managed to build the planner in debug mode on windows and executing the unit tests:

'prost.exe' (Win32): Loaded 'C:\Windows\System32\bcryptprimitives.dll'. 
[doctest] doctest version is "2.3.5"
[doctest] run with "--dt-help" for options
===============================================================================
[doctest] test cases:      3 |      3 passed |      0 failed |      0 skipped
[doctest] assertions:   1087 |   1087 passed |      0 failed |
[doctest] Status: SUCCESS!
'prost.exe' (Win32): Loaded 'C:\Windows\System32\kernel.appcore.dll'. 
The thread 0x270c has exited with code 1 (0x1).
The thread 0x359c has exited with code 1 (0x1).
The program '[15008] prost.exe' has exited with code 1 (0x1).

I did not yet run the planner normally, but what I want to discuss is some of the warning messages which we normally disable and that are automatically enabled in Visual Studio, in particular conversion warnings. Here is the output of the debug build when I compile search with VS2019: https://pastebin.com/uN17eZ0n.

This already excludes some more extreme warnings, such as E:\Repositories\thomaskeller79\prost-dev\src\rddl_parser\utils\string_utils.cc(194): warning C5045: Compiler will insert Spectre mitigation for memory load if /Qspectre switch specified.

Do we want to get rid of these warnings or should we just disable conversion warnings for windows build systems as well? If we do want to get rid of them perhaps creating a separate issue that deals with conversion warnings is more appropriate, because this might induce quite a lot of changes which are not necessarily linked to building the planner under windows.

@thomaskeller79
Copy link
Member Author

Most of these will be fixed by adding static_cast<>() statements, which I am in favor of. After we have updated these, I would even switch to a gcc configuration that doesn't ignore this type of warning.

Let's have a look at what is left after these are fixed, it will be easier then to decide on a case by case basis what to do.

@geisserf
Copy link
Contributor

I will create a separate issue to fix the warnings, since it is not necessarily related to compiling under windows and a separate issue makes it easier to track the changes related only to windows compilation.

@speckdavid
Copy link
Contributor

@geisserf How did you proceed with buddy? I checked the possibility to add a MacOS and Windows test build to github (#104). However, it seems that buddy is only available and "easy" to install within Linux.

@geisserf
Copy link
Contributor

In the old work on this issue by Relić Đorđe Buddy is disabled if it is not found.

Once issue #31 is resolved we can add a Windows test build to github. I would not add a MacOS build as long as we do not explicitly support MacOS as well.

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

No branches or pull requests

3 participants