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

Address Warnings Produced By gempyor Test Suite #244

Open
TimothyWillard opened this issue Jun 28, 2024 · 4 comments · Fixed by #245
Open

Address Warnings Produced By gempyor Test Suite #244

TimothyWillard opened this issue Jun 28, 2024 · 4 comments · Fixed by #245
Assignees
Labels
gempyor Concerns the python core good first issue Good for newcomers

Comments

@TimothyWillard
Copy link
Contributor

TimothyWillard commented Jun 28, 2024

Currently the gempyor test suite throws 28 warnings of 2 types, both of which are deprecation related. "Fixing" this issue wouldn't involve a behavior change, but rather just updating the code to avoid the deprecated behavior/functions. First one is from pandas:

tests/outcomes/test_outcomes.py: 13 warnings
  /Users/twillard/Desktop/GitHub/HopkinsIDD/flepiMoP/flepimop/gempyor_pkg/src/gempyor/outcomes.py:385: FutureWarning: The behavior of DataFrame concatenation with empty or all-NA entries is deprecated. In a future version, this will no longer exclude empty or all-NA columns when determining the result dtypes. To retain the old behavior, exclude the relevant entries before the concat operation.
    hpar = pd.concat(

And second one is from confuse:

tests/utils/test_utils2.py: 15 warnings
  /Users/twillard/Desktop/GitHub/HopkinsIDD/flepiMoP/venv/lib/python3.12/site-packages/confuse/util.py:118: DeprecationWarning: 'pkgutil.get_loader' is deprecated and slated for removal in Python 3.14; use importlib.util.find_spec() instead
    loader = pkgutil.get_loader(name)

I've already addressed the first one, going to take a look at the second one, making an issue for posterity.

@TimothyWillard TimothyWillard added good first issue Good for newcomers gempyor Concerns the python core labels Jun 28, 2024
@TimothyWillard TimothyWillard self-assigned this Jun 28, 2024
@TimothyWillard
Copy link
Contributor Author

Looks like the confuse warnings start with python 3.12 (confirmed this with 3.11 and 3.12) and has already been reported: beetbox/confuse#165. Probably can wait and see on that one.

@KuzeyGorgulu
Copy link

It's good that you've already addressed the first warning related to pandas. For the second warning related to confuse, since it's already been reported and confirmed to start with Python 3.12, it might be worth waiting for the maintainers to update the package. However, if you want to fix it locally in the meantime, you can modify the confuse code yourself or monkey-patch it in your project.

-First step is: Identify the location of the warning, the warning comes from the util.py file in the confuse package.

-Second: Modify the code to use importlib.util.find_spec, you can directly edit the confuse source code in your virtual environment or monkey-patch it in your project.

Here's how you might modify the code in util.py:

import importlib.util

def get_loader(name):
    spec = importlib.util.find_spec(name)
    if spec is None:
        return None
    return spec.loader

Monkey-patch in your project: If you prefer not to modify the package directly, you can add a monkey patch in your project's initialization code.

import importlib.util
import confuse.util

def patched_get_loader(name):
    spec = importlib.util.find_spec(name)
    if spec is None:
        return None
    return spec.loader

confuse.util.get_loader = patched_get_loader

So, for the confuse warning, either wait for the upstream fix or implement a local fix using importlib.util.find_spec.

@TimothyWillard
Copy link
Contributor Author

Hey @KuzeyGorgulu and thank you for the suggestions!

I can't endorse the first approach because modifying a virtualenv manually makes it difficult to have reproducible installs and those changes are liable to be clobbered by modifications to the dependencies. I think the second approach isn't necessary for now since it is easy enough to change the version used by virtualenv and it only presents as a warning in 3.12. I'd rather wait to see what the maintainers of confuse do and react to their decisions on what to do for now.

I think perhaps this code change would be better off packaged as a PR to the confuse package to address the underlying issue. I do not see contribution guidelines, but several successful prior PRs can be used as a guide.

@KuzeyGorgulu
Copy link

I understand your concerns about modifying the virtual environment and the importance of reproducibility. However, I believe my approach is beneficial for a few reasons. Implementing a monkey-patch is a temporary, non-invasive fix that cleans up the deprecation warnings without altering the virtual environment or dependencies, ensuring our development process isn't disrupted. Waiting for the confuse maintainers to address this could take time, during which our logs will be cluttered with warnings. By applying a local fix now, we can keep things running smoothly while we submit a PR to confuse and contribute to the open-source community. This way, we balance immediate needs with long-term solutions. Plus, documenting our local fix ensures transparency and clarity for future developers. Switching Python versions might seem like an easy workaround, but it could introduce other issues if we rely on specific features or fixes in Python 3.12. My approach keeps everything stable and forward-compatible without causing additional headaches.

I am a starter developer right now, so i apologize in case if I misunderstood anything.

@TimothyWillard TimothyWillard linked a pull request Jul 19, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gempyor Concerns the python core good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants