Skip to content

Commit

Permalink
Update use of various deprecated APIs (#4719)
Browse files Browse the repository at this point in the history
This replaces the use of various deprecated APIs pointed out by warnings
thrown during runs of the test suite.
It also introduces one new feature and a bug fix.

Features:

 * Add non-zero exit code for failure to most `verdi daemon` commands, 
    so tests will catch possible errors.

Bug fixes:

* A couple of files were opened but not closed

Updates of deprecated APIs:

* np.int is deprecated alias of int

* np.float is deprecated alias of float

* put_object_from_filelike: force is deprecated

* archive import/export:  `silent` keyword is deprecated in favor of logger

* computer name => label

* Fix tests writing to the repository of nodes after they had been stored
  by replacing all times we use `.open` with `'w'` or `'wb'` mode
  with a correct call to `put_object_from_filelike` *before* the node is stored.

In one case, the data comes from a small archive file. In this case,
I recreated the (zipped) .aiida file adding two additional (binary) files
obtained by gzipping a short string.
This was used to ensure that `inputcat` and `outputcat` work also
when binary data was requested. Actually, this is better than before,
where the actual input or output of the calculation were overwritten
and then replaced back.

* communicator: replace deprecated stop() by close()

* silence some deprecation warnings in tests of APIs that will be removed in 2.0

Note that while unmuting the `ResourceWarning` was good to spot
some issues (bug fix above), the warning is raised in a couple more 
places where it's less obvious to fix (typically related to the daemon
starting some process in the background - or being started itself -
and not being stopped before the test actually finished).
I think this is an acceptable compromise - maybe we'll figure out
how to selectively silence those, and keeping warnings visible will
help us figure out possible leaks in the future.

Co-authored-by: Giovanni Pizzi <giovanni.pizzi@epfl.ch>
  • Loading branch information
ltalirz and giovannipizzi authored Feb 9, 2021
1 parent e99227b commit 6b6481d
Show file tree
Hide file tree
Showing 42 changed files with 477 additions and 363 deletions.
2 changes: 1 addition & 1 deletion aiida/calculations/transfer.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def validate_transfer_inputs(inputs, _):

for node_label, node_object in source_nodes.items():
if isinstance(node_object, orm.RemoteData):
if computer.name != node_object.computer.name:
if computer.label != node_object.computer.label:
error_message = (
f' > remote node `{node_label}` points to computer `{node_object.computer}`, '
f'not the one being used (`{computer}`)'
Expand Down
39 changes: 31 additions & 8 deletions aiida/cmdline/commands/cmd_daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ def start(foreground, number):
If the NUMBER of desired workers is not specified, the default is used, which is determined by the configuration
option `daemon.default_workers`, which if not explicitly changed defaults to 1.
Returns exit code 0 if the daemon is OK, non-zero if there was an error.
"""
from aiida.engine.daemon.client import get_daemon_client

Expand All @@ -78,7 +80,9 @@ def start(foreground, number):
time.sleep(1)
response = client.get_status()

print_client_response_status(response)
retcode = print_client_response_status(response)
if retcode:
sys.exit(retcode)


@verdi_daemon.command()
Expand Down Expand Up @@ -115,24 +119,34 @@ def status(all_profiles):
@click.argument('number', default=1, type=int)
@decorators.only_if_daemon_running()
def incr(number):
"""Add NUMBER [default=1] workers to the running daemon."""
"""Add NUMBER [default=1] workers to the running daemon.
Returns exit code 0 if the daemon is OK, non-zero if there was an error.
"""
from aiida.engine.daemon.client import get_daemon_client

client = get_daemon_client()
response = client.increase_workers(number)
print_client_response_status(response)
retcode = print_client_response_status(response)
if retcode:
sys.exit(retcode)


@verdi_daemon.command()
@click.argument('number', default=1, type=int)
@decorators.only_if_daemon_running()
def decr(number):
"""Remove NUMBER [default=1] workers from the running daemon."""
"""Remove NUMBER [default=1] workers from the running daemon.
Returns exit code 0 if the daemon is OK, non-zero if there was an error.
"""
from aiida.engine.daemon.client import get_daemon_client

client = get_daemon_client()
response = client.decrease_workers(number)
print_client_response_status(response)
retcode = print_client_response_status(response)
if retcode:
sys.exit(retcode)


@verdi_daemon.command()
Expand All @@ -154,7 +168,10 @@ def logshow():
@click.option('--no-wait', is_flag=True, help='Do not wait for confirmation.')
@click.option('--all', 'all_profiles', is_flag=True, help='Stop all daemons.')
def stop(no_wait, all_profiles):
"""Stop the daemon."""
"""Stop the daemon.
Returns exit code 0 if the daemon was shut down successfully (or was not running), non-zero if there was an error.
"""
from aiida.engine.daemon.client import get_daemon_client

config = get_config()
Expand Down Expand Up @@ -190,7 +207,9 @@ def stop(no_wait, all_profiles):
if response['status'] == client.DAEMON_ERROR_NOT_RUNNING:
click.echo('The daemon was not running.')
else:
print_client_response_status(response)
retcode = print_client_response_status(response)
if retcode:
sys.exit(retcode)


@verdi_daemon.command()
Expand All @@ -205,6 +224,8 @@ def restart(ctx, reset, no_wait):
By default will only reset the workers of the running daemon. After the restart the same amount of workers will be
running. If the `--reset` flag is passed, however, the full daemon will be stopped and restarted with the default
number of workers that is started when calling `verdi daemon start` manually.
Returns exit code 0 if the result is OK, non-zero if there was an error.
"""
from aiida.engine.daemon.client import get_daemon_client

Expand All @@ -230,7 +251,9 @@ def restart(ctx, reset, no_wait):
response = client.restart_daemon(wait)

if wait:
print_client_response_status(response)
retcode = print_client_response_status(response)
if retcode:
sys.exit(retcode)


@verdi_daemon.command(hidden=True)
Expand Down
2 changes: 1 addition & 1 deletion aiida/cmdline/commands/cmd_process.py
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ def _print(communicator, body, sender, subject, correlation_id): # pylint: disa
echo.echo('') # add a new line after the interrupt character
echo.echo_info('received interrupt, exiting...')
try:
communicator.stop()
communicator.close()
except RuntimeError:
pass

Expand Down
2 changes: 1 addition & 1 deletion aiida/cmdline/commands/cmd_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def verdi_status(print_traceback, no_rmq):
with Capturing(capture_stderr=True):
with override_log_level(): # temporarily suppress noisy logging
comm = manager.create_communicator(with_orm=False)
comm.stop()
comm.close()
except Exception as exc:
message = f'Unable to connect to rabbitmq with URL: {profile.get_rmq_url()}'
print_status(ServiceStatus.ERROR, 'rabbitmq', message, exception=exc, print_traceback=print_traceback)
Expand Down
18 changes: 12 additions & 6 deletions aiida/cmdline/utils/daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,29 @@ def print_client_response_status(response):
Print the response status of a call to the CircusClient through the DaemonClient
:param response: the response object
:return: an integer error code; non-zero means there was an error (FAILED, TIMEOUT), zero means OK (OK, RUNNING)
"""
from aiida.engine.daemon.client import DaemonClient

if 'status' not in response:
return
return 1

if response['status'] == 'active':
click.secho('RUNNING', fg='green', bold=True)
elif response['status'] == 'ok':
return 0
if response['status'] == 'ok':
click.secho('OK', fg='green', bold=True)
elif response['status'] == DaemonClient.DAEMON_ERROR_NOT_RUNNING:
return 0
if response['status'] == DaemonClient.DAEMON_ERROR_NOT_RUNNING:
click.secho('FAILED', fg='red', bold=True)
click.echo('Try to run \'verdi daemon start --foreground\' to potentially see the exception')
elif response['status'] == DaemonClient.DAEMON_ERROR_TIMEOUT:
return 2
if response['status'] == DaemonClient.DAEMON_ERROR_TIMEOUT:
click.secho('TIMEOUT', fg='red', bold=True)
else:
click.echo(response['status'])
return 3
# Unknown status, I will consider it as failed
click.echo(response['status'])
return -1


def get_daemon_status(client):
Expand Down
8 changes: 6 additions & 2 deletions aiida/engine/daemon/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,9 @@ def get_circus_socket_directory(self) -> str:
"""
if self.is_daemon_running:
try:
return open(self.circus_socket_file, 'r', encoding='utf8').read().strip()
with open(self.circus_socket_file, 'r', encoding='utf8') as fhandle:
content = fhandle.read().strip()
return content
except (ValueError, IOError):
raise RuntimeError('daemon is running so sockets file should have been there but could not read it')
else:
Expand All @@ -208,7 +210,9 @@ def get_daemon_pid(self) -> Optional[int]:
"""
if os.path.isfile(self.circus_pid_file):
try:
return int(open(self.circus_pid_file, 'r', encoding='utf8').read().strip())
with open(self.circus_pid_file, 'r', encoding='utf8') as fhandle:
content = fhandle.read().strip()
return int(content)
except (ValueError, IOError):
return None
else:
Expand Down
2 changes: 1 addition & 1 deletion aiida/manage/tests/pytest_fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def aiida_localhost(temp_dir):
Usage::
def test_1(aiida_localhost):
name = aiida_localhost.get_name()
label = aiida_localhost.get_label()
# proceed to set up code or use 'aiida_local_code_factory' instead
Expand Down
3 changes: 2 additions & 1 deletion aiida/orm/computers.py
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,8 @@ def get_configuration(self, user=None):

config = {}
try:
authinfo = backend.authinfos.get(self, user)
# Need to pass the backend entity here, not just self
authinfo = backend.authinfos.get(self._backend_entity, user)
config = authinfo.get_auth_params()
except exceptions.NotExistent:
pass
Expand Down
4 changes: 2 additions & 2 deletions aiida/orm/nodes/data/array/kpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ def _validate_kpoints_weights(self, kpoints, weights):
else:
raise ValueError(f'kpoints must be a list of lists in {self._dimension}D case')

if kpoints.dtype != numpy.dtype(numpy.float):
if kpoints.dtype != numpy.dtype(float):
raise ValueError(f'kpoints must be an array of type floats. Found instead {kpoints.dtype}')

if kpoints.shape[1] < self._dimension:
Expand All @@ -385,7 +385,7 @@ def _validate_kpoints_weights(self, kpoints, weights):
weights = numpy.array(weights)
if weights.shape[0] != kpoints.shape[0]:
raise ValueError(f'Found {weights.shape[0]} weights but {kpoints.shape[0]} kpoints')
if weights.dtype != numpy.dtype(numpy.float):
if weights.dtype != numpy.dtype(float):
raise ValueError(f'weights must be an array of type floats. Found instead {weights.dtype}')

return kpoints, weights
Expand Down
3 changes: 2 additions & 1 deletion aiida/transports/plugins/ssh.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ def parse_sshconfig(computername):
import paramiko
config = paramiko.SSHConfig()
try:
config.parse(open(os.path.expanduser('~/.ssh/config'), encoding='utf8'))
with open(os.path.expanduser('~/.ssh/config'), encoding='utf8') as fhandle:
config.parse(fhandle)
except IOError:
# No file found, so empty configuration
pass
Expand Down
2 changes: 2 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ filterwarnings = [
"ignore::DeprecationWarning:jsonbackend:",
"ignore::DeprecationWarning:reentry:",
"ignore::DeprecationWarning:pkg_resources:",
"ignore::pytest.PytestCollectionWarning",
"default::ResourceWarning",
]
markers = [
"sphinx: set parameters for the sphinx `app` fixture"
Expand Down
39 changes: 19 additions & 20 deletions tests/cmdline/commands/test_calcjob.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,13 @@ def test_calcjob_inputls(self):
options = [self.arithmetic_job.uuid]
result = self.cli_runner.invoke(command.calcjob_inputls, options)
self.assertIsNone(result.exception, result.output)
self.assertEqual(len(get_result_lines(result)), 3)
# There is also an additional fourth file added by hand to test retrieval of binary content
# see comments in test_calcjob_inputcat
self.assertEqual(len(get_result_lines(result)), 4)
self.assertIn('.aiida', get_result_lines(result))
self.assertIn('aiida.in', get_result_lines(result))
self.assertIn('_aiidasubmit.sh', get_result_lines(result))
self.assertIn('in_gzipped_data', get_result_lines(result))

options = [self.arithmetic_job.uuid, '.aiida']
result = self.cli_runner.invoke(command.calcjob_inputls, options)
Expand All @@ -156,10 +159,13 @@ def test_calcjob_outputls(self):
options = [self.arithmetic_job.uuid]
result = self.cli_runner.invoke(command.calcjob_outputls, options)
self.assertIsNone(result.exception, result.output)
self.assertEqual(len(get_result_lines(result)), 3)
# There is also an additional fourth file added by hand to test retrieval of binary content
# see comments in test_calcjob_outputcat
self.assertEqual(len(get_result_lines(result)), 4)
self.assertIn('_scheduler-stderr.txt', get_result_lines(result))
self.assertIn('_scheduler-stdout.txt', get_result_lines(result))
self.assertIn('aiida.out', get_result_lines(result))
self.assertIn('gzipped_data', get_result_lines(result))

options = [self.arithmetic_job.uuid, 'non-existing-folder']
result = self.cli_runner.invoke(command.calcjob_inputls, options)
Expand All @@ -186,16 +192,13 @@ def test_calcjob_inputcat(self):
self.assertEqual(get_result_lines(result)[0], '2 3')

# Test cat binary files
with self.arithmetic_job.open('aiida.in', 'wb') as fh_out:
fh_out.write(gzip.compress(b'COMPRESS'))

options = [self.arithmetic_job.uuid, 'aiida.in']
# I manually added, in the export file, in the files of the arithmetic_job,
# a file called 'in_gzipped_data' whose content has been generated with
# with open('in_gzipped_data', 'wb') as f:
# f.write(gzip.compress(b'COMPRESS-INPUT'))
options = [self.arithmetic_job.uuid, 'in_gzipped_data']
result = self.cli_runner.invoke(command.calcjob_inputcat, options)
assert gzip.decompress(result.stdout_bytes) == b'COMPRESS'

# Replace the file
with self.arithmetic_job.open('aiida.in', 'w') as fh_out:
fh_out.write('2 3\n')
assert gzip.decompress(result.stdout_bytes) == b'COMPRESS-INPUT'

def test_calcjob_outputcat(self):
"""Test verdi calcjob outputcat"""
Expand All @@ -217,18 +220,14 @@ def test_calcjob_outputcat(self):
self.assertEqual(get_result_lines(result)[0], '5')

# Test cat binary files
retrieved = self.arithmetic_job.outputs.retrieved
with retrieved.open('aiida.out', 'wb') as fh_out:
fh_out.write(gzip.compress(b'COMPRESS'))

options = [self.arithmetic_job.uuid, 'aiida.out']
# I manually added, in the export file, in the files of the output retrieved node of the arithmetic_job,
# a file called 'gzipped_data' whose content has been generated with
# with open('gzipped_data', 'wb') as f:
# f.write(gzip.compress(b'COMPRESS'))
options = [self.arithmetic_job.uuid, 'gzipped_data']
result = self.cli_runner.invoke(command.calcjob_outputcat, options)
assert gzip.decompress(result.stdout_bytes) == b'COMPRESS'

# Replace the file
with retrieved.open('aiida.out', 'w') as fh_out:
fh_out.write('5\n')

def test_calcjob_cleanworkdir(self):
"""Test verdi calcjob cleanworkdir"""

Expand Down
Loading

0 comments on commit 6b6481d

Please sign in to comment.