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

Fix ResourceWarning due to unclosed file #212

Merged
merged 3 commits into from
Sep 18, 2023

Conversation

kkovary
Copy link
Contributor

@kkovary kkovary commented Sep 18, 2023

Changelogs

closes #211

  • enumerate the changes of that PR.
    This PR addresses a ResourceWarning that was being raised due to an unclosed file in datamol/mol.py. The warning was triggered by the following line of code:
SALT_SOLVENT_PATH = datamol.data.open_datamol_data_file("salts_solvents.smi").name

This line opens a file but does not close it, which is why the ResourceWarning was being raised. To resolve this issue, I've modified the line to use a with block, which ensures that the file is properly closed after its name is retrieved:

with datamol.data.open_datamol_data_file("salts_solvents.smi") as file_obj:
    SALT_SOLVENT_PATH = file_obj.name

This change should prevent the ResourceWarning from being raised in the future. I've tested the change locally and confirmed that it resolves the warning without causing any other issues.


Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the new introduced feature(s) (if appropriate).
  • Update the API documentation is a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs below.
  • If possible, assign one of the following labels to the PR: feature, fix or test (or ask a maintainer to do it for you).

discussion related to that PR

@kkovary kkovary requested a review from hadim as a code owner September 18, 2023 04:29
@kkovary kkovary changed the title Fix ResourceWarning by properly closing file Fix ResourceWarning due to unclosed file Sep 18, 2023
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #212 (4bf712c) into main (22b5bb0) will decrease coverage by 0.04%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
- Coverage   91.95%   91.91%   -0.04%     
==========================================
  Files          46       46              
  Lines        3827     3835       +8     
==========================================
+ Hits         3519     3525       +6     
- Misses        308      310       +2     
Flag Coverage Δ
unittests 91.91% <77.77%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
datamol/data/__init__.py 78.07% <75.00%> (-0.24%) ⬇️
datamol/mol.py 97.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@hadim
Copy link
Contributor

hadim commented Sep 18, 2023

Thank you for the catch! This is indeed a bug, but I don't think we should even open that file to get its path. Instead we should simply retrieve the path.

In datamol/data/__init__.py, can you add the below function and use it in mol.py with SALT_SOLVENT_PATH = datamol_data_file_path("salts_solvents.smi")?

@functools.lru_cache()
def datamol_data_file_path(filename: str, dm_module: str = "datamol.data"):
    if sys.version_info < (3, 9, 0):
        with importlib_resources.path("datamol.data", filename) as p:
            data_path = p
    else:
        data_path = importlib_resources.files(dm_module).joinpath(filename)

    return str(data_path)

@hadim hadim added the fix label Sep 18, 2023
@kkovary
Copy link
Contributor Author

kkovary commented Sep 18, 2023

Thanks for the suggestion @hadim, that's definitely a much better way of handling things. I'll push an update a little later today.

@kkovary
Copy link
Contributor Author

kkovary commented Sep 18, 2023

@hadim I just pushed a commit with the requested changes.

datamol/data/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hadim hadim left a comment

Choose a reason for hiding this comment

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

Thank you. See a small comment below.

@kkovary kkovary requested a review from hadim September 18, 2023 20:35
@hadim
Copy link
Contributor

hadim commented Sep 18, 2023

Thank you, @kkovary, for your contribution!

@hadim hadim merged commit e3c4a38 into datamol-io:main Sep 18, 2023
16 checks passed
@kkovary
Copy link
Contributor Author

kkovary commented Sep 18, 2023

Thank you @hadim for the wonderful tools, hope to make more contributions in the future 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve ResourceWarning due to unclosed file in datamol/mol.py
2 participants