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.

Sign up for GitHub

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

Jump to bottom

Fix the with_daemon test fixture and suppress unclosed zmq socket warnings #5631

Merged
merged 4 commits into from
Sep 12, 2022
Merged

Fix the with_daemon test fixture and suppress unclosed zmq socket warnings #5631

merged 4 commits into from
Sep 12, 2022

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 5, 2022 •
edited
Loading

Fixes #5628

@sphuber sphuber force-pushed the fix/5628/with-daemon-fixture branch 2 times, most recently from cdd3b09 to 8ec390f Compare September 5, 2022 20:54
@sphuber sphuber requested a review from ltalirz September 5, 2022 21:25
@sphuber
Copy link
Contributor Author

sphuber commented Sep 5, 2022

This is a prerequisite for #5630 that will add new API for process control

ltalirz
ltalirz reviewed Sep 5, 2022
View reviewed changes
Copy link
Member

@ltalirz ltalirz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks a lot for the fix @sphuber !

When you say "the current implementation", do we know when this broke (or whether it was always like this)? This would be important to mention in the commit message.

And would it be possible to create a test for this?

aiida/manage/configuration/profile.py Show resolved Hide resolved
aiida/manage/tests/main.py Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@sphuber
Copy link
Contributor Author

sphuber commented Sep 6, 2022

When you say "the current implementation", do we know when this broke (or whether it was always like this)? This would be important to mention in the commit message.

The current with_daemon fixture was only used by a single test tests/cmdline/commands/test_process.py::test_pause_play_kill which was actually skipped (see #4731). The issue claims that it was an occasional breaking test and not consistently, so I am not sure what the exact reason was. Also, the bug that I am addressing here would only surface if used with an automatically generated temporary config and profile for the test. When using with a preconfigured test profile in the normal config directory, it should probably work. Although the new fixtures now nicely use the new DaemonClient API and make sure to stop the daemon process at the end of the session.

And would it be possible to create a test for this?

Not sure how to do that. Just testing that we can start and stop the daemon with the fixtures and reach it is not enough, because the bug only surface when using a temp test profile. With a preconfigured one it never showed. But not sure how to enforce a temp profile if people launch the test suite with a preconfigured profile.

@ltalirz
Copy link
Member

ltalirz commented Sep 6, 2022

All good!
I somehow glanced over the title of the issue and did not realize it is really just localized to the fixture.

Feel free to merge after changing the names, no further review needed from my side.

@sphuber sphuber force-pushed the fix/5628/with-daemon-fixture branch 23 times, most recently from 48ea00c to 2e4d316 Compare September 6, 2022 15:40
@sphuber sphuber force-pushed the fix/5628/with-daemon-fixture branch 4 times, most recently from 3976706 to 7971b1c Compare September 7, 2022 07:56
@sphuber sphuber requested a review from ltalirz September 7, 2022 09:03
@sphuber
Copy link
Contributor Author

sphuber commented Sep 7, 2022

As you may have noticed @ltalirz , I have been working quite a bit more on this, as I wasn't fully satisfied and things weren't fully robust. I have separated out a few more commits to make it clearer what the prerequisite work was. It is now ready for a final review.

The crucial differences compared to your last review are:

  • Add env['PYTHONPATH'] = ':'.join(sys.path) to the DaemonClient.get_env method. This guarantees that the daemon will be able to import everything (such as workchains etc.)
  • Add a timeout check to start_daemon and stop_daemon to make sure that is_daemon_running returns the correct value. This is crucial for the started_daemon_client and stopped_daemon_client to provide a guarantee of the daemon status that the test can then rely on.

@sphuber sphuber force-pushed the fix/5628/with-daemon-fixture branch 4 times, most recently from 99d3e4d to c0d7624 Compare September 7, 2022 16:02
@sphuber
Copy link
Contributor Author

sphuber commented Sep 7, 2022

@ltalirz applied the fixes we discussed online. I messed around a bit with the signal module and how to signal to the parent process from the child process when it is done, but it is not obvious and could not get it to work immediately. I would suggest to leave that for another time since we are not changing the status quo here anyway.

ltalirz
ltalirz reviewed Sep 12, 2022
View reviewed changes
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @sphuber !

just some minor comments/questions remaining

aiida/cmdline/utils/common.py Outdated Show resolved Hide resolved
aiida/engine/daemon/client.py Outdated Show resolved Hide resolved
aiida/engine/daemon/client.py Show resolved Hide resolved
tests/cmdline/commands/test_process.py Show resolved Hide resolved
aiida/engine/daemon/client.py Show resolved Hide resolved
tests/cmdline/commands/test_process.py Show resolved Hide resolved
@sphuber sphuber mentioned this pull request Sep 12, 2022
Closed
@sphuber
Profile: make definition of daemon filepaths dynamic
c9ee1cd 
The filepaths relevant to the daemon that are provided through the
`Profile.filepaths` property, where defined statically at the module
level. This means that they would be fixed as soon as the `profile`
module would be first imported based on the `DAEMON_DIR` and the
`DAEMON_LOG_DIR` variables of the `settings` module.

This would be problematic if those variables were to be changed, because
they wouldn't be reflected by `Profile.filepaths`. An example of such a
case is the running of the test suite without a pre-configured test
profile. In this case, a temporary test profile is created from scratch
including the AiiDA configuration directory. Since the path of the
configuration directory changes, the filepaths of the daemon also
changed and this would have to be reflected by `Profile.filepaths`.
@sphuber
DaemonClient: add the get_env method
413b8c6 
The `DaemonClient.get_env` method is added which provides a copy of the
environment that is used by the process in which the client is running.
This is used by the `start_daemon` and `_start_daemon` methods to ensure
that the launched subprocesses use the exact same environment as the
process of the client.

The implementation is largely taken from the `get_env_with_venv_bin`
function of the `aiida.cmdline.utils.common` utility. This function is
deprecated since its is purely relevant to the daemon functionality and
so should just be contained in the `DaemonClient`.

There is a single important change and that is that `get_env` adds the
current content of `sys.path` to the `PYTHONPATH` key. This is important
as to guarantee that the daemon process will have the same module paths
as the main process. This ensures that all modules that are importable
by the main process, will also be importable by the daemon process.
@sphuber
DaemonClient: close CircusClient after call
0171a78 
The `CircusClient` opens a `zmq` socket which wasn't being closed. By
transforming `DaemonClient.client` into a context manager that
automatically calls `client.stop()` at the end, the socket is closed
properly. This gets rid of the multitude of `ResourceWarnings` at the
end of the test suite.
@sphuber
Fixtures: Add started_daemon_client and stopped_daemon_client
95eb1e5 
The fixtures `started_daemon_client` and `stopped_daemon_client` are
added which are function scoped fixtures that ensure that the daemon is
running or stopped, respectively. The fixtures return an instance of the
`DaemonClient` which can be used to interact further with the daemon
where necessary.

The fixtures both use the `daemon_client` fixture which is session
scoped and will ensure that the daemon is stopped at the end of the test
session. The `started_daemon_client` replaces the `with_daemon` fixture
which was not going through the `DaemonClient` designed for this
purpose.
@sphuber sphuber merged commit 77a18d0 into aiidateam:main Sep 12, 2022
@sphuber sphuber deleted the fix/5628/with-daemon-fixture branch September 12, 2022 13:32
@sphuber sphuber mentioned this pull request Sep 21, 2022
Closed
@AhmedBasem20 AhmedBasem20 mentioned this pull request Mar 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ltalirz ltalirz ltalirz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix the with_daemon fixture when running with temporary profile
2 participants
@sphuber @ltalirz