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

Devops: Address deprecation warnings from internal code and dependencies #6272

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Feb 1, 2024

This addresses the majority of deprecation warnings that are emitted under Python 3.9. The Python 3.12 run has quite a number more but they are mostly caused by dependencies and so they will need updates upstream. These will be taken care of at a later point in time.

The remaining warnings are the following:

 tests/test_dataclasses.py::TestCifData::test_ase_primitive_and_conventional_cells_ase
  /opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/site-packages/ase/atoms.py:1205: DeprecationWarning: `product` is deprecated as of NumPy 1.25.0, and will be removed in NumPy 2.0. Please use `prod` instead.
    atoms *= rep

This should be fixed by ase.

tests/cmdline/commands/test_code.py::test_code_duplicate_non_interactive[vim -cwq]
  /home/runner/work/aiida-core/aiida-core/src/aiida/orm/utils/builders/code.py:17: AiidaDeprecationWarning: This module is deprecated. To create a new code instance, simply use the constructor. (this will be removed in v3)
    warn_deprecation('This module is deprecated. To create a new code instance, simply use the constructor.', version=3)

This is because verdi code duplicate still requires the deprecated CodeBuilder. Addressing this will require quite some work and so is reserved for a separate PR.

tests/cmdline/commands/test_code.py::test_code_export
tests/cmdline/commands/test_code.py::test_code_create[sleep 1; vim -cwq-core.code.installed]
tests/cmdline/commands/test_code.py::test_code_create[sleep 1; vim -cwq-core.code.portable]
tests/cmdline/commands/test_code.py::test_code_create[sleep 1; vim -cwq-core.code.containerized]
  /home/runner/work/aiida-core/aiida-core/src/aiida/cmdline/groups/dynamic.py:145: AiidaDeprecationWarning: Relying on `_get_cli_options` is deprecated. The options should be defined through a `pydantic.BaseModel` that should be assigned to the `Config` class attribute. (this will be removed in v3)
    warn_deprecation(

This would be taken care of by #6190

tests/engine/test_work_chain.py::TestWorkchain::test_tocontext_schedule_workchain
tests/engine/processes/calcjobs/test_calc_job.py::test_multi_codes_with_mpi[code_info_with_mpi_none-None-None-False-0]
  /opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/asyncio/base_events.py:686: ResourceWarning: unclosed event loop <_UnixSelectorEventLoop running=False closed=False debug=False>
    _warn(f"unclosed event loop {self!r}", ResourceWarning, source=self)
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

tests/engine/processes/calcjobs/test_calc_job.py::test_multi_codes_with_mpi[code_info_with_mpi_none-None-None-False-0]
  /opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/asyncio/base_events.py:672: RuntimeWarning: coroutine 'Process.step_until_terminated' was never awaited
    self._ready.clear()
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

These are due to the test test_call_on_process_finish in tests/engine/test_runners.py. The fix would be to close the new event loop that is created in the runner fixture, or simply use the Runner class to take care of it. This works for Python 3.9, however, it fails for Python 3.12. The behavior of asyncio.get_event_loop changed in Python 3.12 where now it raises if no event loop has been set. This first needs to be addressed in plumpy before we can fix it in aiida-core.

@sphuber sphuber force-pushed the fix/deprecation-warnings branch 3 times, most recently from 84d3bb6 to ba2fc58 Compare February 2, 2024 09:53
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (35d7ca6) 77.70% compared to head (911b1f2) 77.64%.
Report is 2 commits behind head on main.

Files Patch % Lines
src/aiida/transports/util.py 57.15% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6272      +/-   ##
==========================================
- Coverage   77.70%   77.64%   -0.05%     
==========================================
  Files         548      548              
  Lines       40230    40237       +7     
==========================================
- Hits        31255    31239      -16     
- Misses       8975     8998      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sphuber sphuber force-pushed the fix/deprecation-warnings branch from 911b1f2 to 57dfd79 Compare February 8, 2024 15:23
@sphuber sphuber changed the title Devops: Address internal deprecation warnings Devops: Address deprecation warnings from internal code and dependencies Feb 8, 2024
@sphuber sphuber requested a review from unkcpz February 8, 2024 22:50
@sphuber sphuber force-pushed the fix/deprecation-warnings branch 4 times, most recently from c61c54a to ed578ae Compare February 13, 2024 14:33
Following package requirements are updated:

 * `coverage~=7.0`
 * `pytest-cov~=4.1`
 * `pympler~=1.0`
 * `pyparsing~=3.0`
This prevents leaving open event loops which cause `ResourceWarnings` to
be emitted during tests.
@sphuber sphuber force-pushed the fix/deprecation-warnings branch from ed578ae to 3fc0dfa Compare February 13, 2024 16:41
@sphuber sphuber requested a review from mbercx February 13, 2024 16:55
@sphuber
Copy link
Contributor Author

sphuber commented Feb 13, 2024

@unkcpz and @mbercx this is ready for review.

@unkcpz
Copy link
Member

unkcpz commented Feb 13, 2024

I'll take a look later. FYI, the docker test failed because reach the rate limit.

Copy link
Member

@unkcpz unkcpz 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 one small question, the rest looks all good.

Comment on lines +72 to +73
except ValueError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why ignore the ValueError specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be raised iof the file handle was already closed, which can happen

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks.

@unkcpz
Copy link
Member

unkcpz commented Feb 14, 2024

Also since this PR is on deprecation, would you mind to take a look at the deprecation in doc buildhttps://readthedocs.org/projects/aiida-core/builds/23431980/ ? I assume it will follow the same rules of change to get rid of those.

@sphuber
Copy link
Contributor Author

sphuber commented Feb 14, 2024

Also since this PR is on deprecation, would you mind to take a look at the deprecation in doc buildhttps://readthedocs.org/projects/aiida-core/builds/23431980/ ? I assume it will follow the same rules of change to get rid of those.

You mean the warn_deprecation( messages in the build log? Those are just there because it imports the code to do the auto API docs and so it hits our own deprecation warnings. There is not much we can do there, can we?

@unkcpz
Copy link
Member

unkcpz commented Feb 14, 2024

Those are just there because it imports the code to do the auto API docs and so it hits our own deprecation warnings. There is not much we can do there, can we?

I see your point, if the warnings are from import InstalledCode which inherit the Code and the warning will anyway raised, correct? Then, I don't think there is much we can do.

But I think this one show in the doc should be suppressed.

image

The `get_description` method was unjustly deprecated in favor of the
`description` property. However, the latter is just a direct reference
to the `description` column of the database model. This is an optional
string that can be defined by the user when the instance is created.
 The `get_description` method rather, which is defined on the `Node`
base class, is intended to return a description composed from relevant
properties of the node. These are therefore not the same.

The deprecation is undone and it now returns `Code.full_label`.
@sphuber
Copy link
Contributor Author

sphuber commented Feb 14, 2024

But I think this one show in the doc should be suppressed.

Ah yeah, that one should be fixed. This is really an incorrect deprecation, so I removed the deprecation altogether.

How did you find this deprecation warning? Just by going through the built docs and you happened to stumble on it when reading the tutorial? Or is there an easier way to find all deprecations? The docs build log contains a bunch of hits of our warn_deprecation but I don't understand where they are coming from. The logs seem all mangled and are impossible to read.

@unkcpz
Copy link
Member

unkcpz commented Feb 14, 2024

Just by going through the built docs and you happened to stumble on it when reading the tutorial?

yep 😁. You know I translate doc to Chinese, I spot in when review the auto-translation PR.

The logs seem all mangled and are impossible to read.

True, it is hard to read, I neither have clue on how to fix. But if the sphinx command run locally, the output is well indented with line breaks. Might be a bit easy to read. I think all those warnings are raised during import phase in apidoc build. Thus not too much to check and fix.

@sphuber sphuber merged commit 1b13014 into aiidateam:main Feb 14, 2024
38 checks passed
@sphuber
Copy link
Contributor Author

sphuber commented Feb 14, 2024

Thanks for the review @unkcpz

@sphuber sphuber deleted the fix/deprecation-warnings branch February 14, 2024 21:29
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.

2 participants