test examples #19
Comments
|
AgentComponent.py gives |
|
Fixed AgentComponent demo |
|
AgentFactory.py |
|
Correct import is |
Strange, did someone rename agentconfig.yaml to agent_config.yaml? |
So it seems someone took over and modified agentconfig.yaml or removed it completely, breaking this example |
|
Trying to use factory_floor_complex.yaml instead |
|
That gives |
|
The yaml file is incorrect/not matching the code. The code expects for each robot something like {'id':'robotname', 'pos':[x,y]}. |
|
Fixed that part of the yaml file. Now we get |
|
The 'agents' value in the yaml file is this which does not even remotely resemble what is needed for the factory (it needs a class name etc) |
|
Basically the yaml files contain a serialized object. What we do is (1) deserialize the yaml to a python dict (2) we have code snippets throughout the code picking parts from this dict (3) build python objects based on these picked parts. Basically we have a distributed hand-built parser that results in a nightmare scenario. You basically have to read through all the code to determine the proper format for your yaml file. Plus all problems you get with simplistic parsing like no /bad error messages. And lots of boilerplate code to fix these bad error messages. I remember we looked into this before but did not find any suited deserialization mechanism for python. Now I see that python 3.8.3 has a dataclass annotation. Maybe we can do something with that? |
|
@Wouter1 perhaps let's just fix outdated config files (adapt them to the new, working format). These config files are not for deserialization but to provide arguments for the constructors/factories (which can do some non-trivial initializations based on these arguments, e.g. creating subclasses). |
|
agent_combined_config.yaml seems to contain something that matches what I reverse engineer in the code. |
|
@czechows yes I'm trying to fix them.But as said this is not nice and they are almost all broken. |
|
Exampel AgentFactory is fixed. |
|
DQNSingleExample.py does not work |
|
The error message comes from searching for a non-existing file. |
|
The used 'scenario_path' is pointing to '/home/wouter/git/aienvs/aienvs/Sumo/../../scenarios/Sumo/grid_1x1' which is completely wrong. There is no Sumo directory in aienvs There is a Sumo dir in aienvs, but there is no grid_1x1 in there there is a aienvs/scenarios/Sumo/one_grid, maybe that's the one intended here |
|
the 'scene' parameter contains just "grid_1x1". And even worse, it's computed in GridSumoEnv, so it's not just a text value in a file. I'll try renaming the one_grid into grid1x1 and see what happens |
|
I'm trying to fix DQNSingleExample. One problem is that it uses aienvs.SumoHelper (so this is a different package that might be just pip install'ed). SumoHelper searches relative to its own file position. I happen to have SumoHelper inside Eclipse, so it givs a file inside aienvs. This is already weird, because the test I'm running is in aiagents. I don't know what would happen if you would have just pip installed aienvs. Should we fix SumoHelper for re-use in other packages? Eg so that it looks in another place ? How? Or do you have a different idea about this? |
|
If I copy the directory aienvs/scenarios/Sumo/one_grid to aienvs/scenarios/Sumo/grid_1x1 then SQNSAingleExample seems to start. Unfortunately the map is incorrect, I get |
|
Maybe I can find the old grid_1x1 files |
How does it use SumoHelper? For me it works (my config is aienvs, aiagents on PYTHONPATH and not installed via pip). Is the problem in GridSumoEnv? |
|
@czechows do you know if we ever had a grid_1x1 sumo scenario folder? The DQNSingleExample needs it |
|
Ah indeed I have this folder locally but not in the repo. Perhaps it is generated by GridSumoEnv? |
|
@czechows Yes I was just thinking that. |
|
I'll skip this for now till we have an idea how to fix this. |
|
@czechows I'm puzzled by the agent_combined_config.yaml that is used for MctsAggrExample MctsAgent is a single agent. That means FAIK that it is connected to a single entity in the env. So it can't have the id "robots", it should have a existing robot name eg robotN with N in [1,2,3]. Also MCTS agent does not seem to be simple because it contains several like treeAgent, rolloutAgent etc? |
|
This is an example to show how to aggregate entities (using some of the functionality you wrote for us some time ago like PackedSpace etc.). Therefore multiple robots are controlled by a single agent. treeAgent, rolloutAgent are specific to internals of MCTS; you have to specify what algorithm of picking actions (=agent) one uses at certain parts within MCTS (e.g. random) |
|
@czechows But then the RobotAgent (MctsAgent) should get a VALID agentid. 'robots' is not a valid ID? |
|
@czechows also, if the MctsAgent is only acting for one entity (say robot1), then who is determining the actions for the other robots? |
|
The MctsExample uses agent_config.yaml which seems to contain a more complete and up-to-date content. Still no 'simulator' here though |
|
@Wouter1 it is acting for all robots at once According to this line the new (combined) id is "robots". |
|
AgentFactory line 56 is weird Why are we calling the constructor multiple times and doing nothing with obj? Only the last value seems returned |
|
It seems that the indentation is incorrect, I assume that obj should be called only when the subAgentList is complete. |
|
Getting in MctsExample The error message is saying that FactoryFloorIterativeGreedy has incorrect constructor, but the errormessage itself is also confusing. The 'self' parameter is not shown. The problem is that it should take actionspace:Dict, observationspace but takes environment. |
|
Fixed that. Now getting |
|
It seems FactoryFloorIterativeGreedy is assuming it can access a 'pathDict' field inside the FactoryFloorAgent. But this is not thhe case. There is a pathDict but it is generated only after step. Also I doubt that it should be visible. |
|
Moved that part of the code to FactoryFloorIterativeGreedy#step |
|
Yes, now we have |
Yes so this code was not used for a while but I remember that it was the only way to engineer it in a sane way. Perhaps pathDict should be an empty pointer in FactoryFloorAgent on initialization? Everything is by reference so it will work well. PathDict being public is OK |
|
The problem is that even in FactoryFloorIterativeGreedy#step we don't always have pathDict in FactoryFloor.pathDict. It seems better to make a function getPathDict() or so so that FactoryFloorAgent can compute it if necessary. |
Well so it does compute it each step for itself anyways. In general I would be careful with any non-trivial refactoring FactoryFloorAgent because test coverage is not great and it is easy to make things very sub-optimal in terms of computation. |
|
Better. Now |
@czechows thanks but I take that as a comment on my fixes; I can not do much with your comment. pathDict was used before it was even computed so the caller's code was broken. I had to clean that up. Can you please answer the open questions I asked earlier? Progress there is blocked awaiting your answers |
No it's not OK. It's not even available until an undefined moment. And if it's not avaialble the caller will use a bogus pathDict with your solution. I fixed this by making an explicit function getPathDict in FactoryFloorAgent that will compute the value if it was not yet computed, and cache it for future use. |
|
@czechows This ticket is MUCH more work than anticipated. Maybe we should make a separate ticket for each example. That also allows us to prioritize the work on this. I worked basically the entire day with just 5 examples and they are not even fully fixed yet. |
|
@Wouter1 which questions do you refer to? It is fine that things are not fully fixed. I didn't know what needed fixing. |
|
@czechows I'm out of time already, just writing a quick update Fixed many things but not nearly enough. Almost all examples are broken and in many ways. @Wouter1 it is acting for all robots at once According to this line the new (combined) id is "robots". Ah I missed that line. Then there must be something else broken that I still missed We have to make separate tickets for each example as the amount of work on each is so much. And we then may have to find back the open ends here and collect them. BTW I put the tickets in aienvs/aiagents but I forgot about them being public. |
That is fine, perhaps even expected. They were not updated during many revisions,
Whether you track it in one or several threads is all fine by me.
Yes they are all public as indicated in my emails from some time ago. |
|
Can you please transfer these? Thanks! Why would it be impossible to make unit tests for these? |
done!
I think examples serve a different purpose than unit tests, and they also sometimes take much longer to finish.. |
|
The unit tests show that the code works. So there's nothing wrong with testing the examples. |
On 03/06/2020 17:12, Aleksander Czechowski wrote:
The text was updated successfully, but these errors were encountered: