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

Appropriately dispose smarts (#378) #665

Merged
merged 7 commits into from
Mar 15, 2021

Conversation

JingfeiPeng
Copy link
Contributor

@JingfeiPeng JingfeiPeng commented Mar 10, 2021

  • Ensured SMARTS destroy can be called safely multiple times
  • Ensured Traffic_history_service clean up sub process

Ensured Investigate to see if SMARTS instance can automatically close on exit
Seems SMARTS instance cannot bt automatically close on exit. When running test.py, if user does not call smarts.destroy(), smarts.del is not called before the next iteration, which causes a "mutiple showbase error". There is no guarantee on del being called immediately after references to smarts is lost. So I think there is a need for users to call destroy() explicitly in smarts.

SMARTS instance is cleaned up when destroyed (e.x. SMARTS.destroy() is called from delete or atexit)
Done

Introduce clear documentation to explain how to dispose of SMARTS instances appropriately.
https://github.com/huawei-noah/SMARTS/blob/master/docs/sim/env.rst seems to have already mentioned to call env.close

Update all examples to follow the appropriate practice.
If we still forces user to call destroy() explicitly, then there is no change needed in examples.

@sah-huawei
Copy link
Contributor

Also, should a smarts.destroy() call be added to more of the examples? (Looks like not all of them use it.)

@JingfeiPeng
Copy link
Contributor Author

JingfeiPeng commented Mar 11, 2021

Also, should a smarts.destroy() call be added to more of the examples? (Looks like not all of them use it.)

We should if the examples don't call smarts.destroy. Though do you have an example of an example that didn't call smarts destroy? The examples under examples directory do call smarts.destroy when they call env.close()

@sah-huawei
Copy link
Contributor

Also, should a smarts.destroy() call be added to more of the examples? (Looks like not all of them use it.)

We should if the examples don't call smarts.destroy. Though do you have an example of an example that didn't call smarts destroy? The examples under examples directory do call smarts.destroy when they call env.close()

I saw one in observation_collection_for_imitation_learning.py but maybe that only exists on my branch?

@JingfeiPeng JingfeiPeng changed the title WIP Appropriately dispose smarts Appropriately dispose smarts Mar 11, 2021
@JingfeiPeng
Copy link
Contributor Author

Also, should a smarts.destroy() call be added to more of the examples? (Looks like not all of them use it.)

We should if the examples don't call smarts.destroy. Though do you have an example of an example that didn't call smarts destroy? The examples under examples directory do call smarts.destroy when they call env.close()

I saw one in observation_collection_for_imitation_learning.py but maybe that only exists on my branch?

Right, that example is missing smarts.destroy. I added it now. Also checked all other examples either calls env.close or smarts.destroy(). The exceptions are rllib.py and rllib_agent.py which don't seem to use smarts or env.

@@ -77,6 +82,20 @@ def __init__(self, history_file_path):
self._prepare_next_batch()
self._receive_data_conn.recv()

def teardown(self):
if self.is_in_use:
self._request_queue.put(Traffic_history_service.QueueDone())
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Not very important in this case but I have had this thought that it might be useful to drain the queue before adding the QueueDone signal.

Copy link
Contributor Author

@JingfeiPeng JingfeiPeng Mar 15, 2021

Choose a reason for hiding this comment

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

while I was trying to drain the queue before adding QueueDone, I noticed the check for QueueDone was not reached in the child process.

        while True:
            print("start")
            historyRange = request_queue.get()
            print("got: ",historyRange)
            if type(historyRange) is Traffic_history_service.QueueDone:
                print("Found done")
                break

In fact, print("got: ",historyRange) was not reached either, then I tried putting other values in the queue and print("got: ",historyRange) still doesn't show up. Though teardown method was reached. Is this related to putting items in a queue in ___del___? I think it is as I tried calling traffic_history_service.teardown in other places and they works, but the documentation for multiprocessing queue didn't seem to mention this

Copy link
Contributor Author

@JingfeiPeng JingfeiPeng Mar 15, 2021

Choose a reason for hiding this comment

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

The reason this happens is __del__ for traffic_history_service happens even after smarts calls destroy, when teardown is called, the child process self._fetch_history_proc is no longer alive..... I think it is because when the program is shutting down, it already closed the child process

@JingfeiPeng JingfeiPeng requested a review from iman512003 March 15, 2021 13:27
@JingfeiPeng JingfeiPeng changed the title Appropriately dispose smarts Appropriately dispose smarts (#378) Mar 15, 2021
@JingfeiPeng JingfeiPeng linked an issue Mar 15, 2021 that may be closed by this pull request
4 tasks
@JingfeiPeng JingfeiPeng merged commit c71fa99 into develop Mar 15, 2021
@JingfeiPeng JingfeiPeng deleted the appropriately_dispose_smarts branch April 13, 2021 22:37
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.

Appropriately dispose of SMARTS at exit
4 participants