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

Missing ModularVisualization in rainfall example? #105

Closed
mrceresa opened this issue Sep 27, 2022 · 14 comments · Fixed by #106
Closed

Missing ModularVisualization in rainfall example? #105

mrceresa opened this issue Sep 27, 2022 · 14 comments · Fixed by #106
Labels
bug Release notes label

Comments

@mrceresa
Copy link

Describe the bug
Cloned the repo and run the coverage tests:

$ pytest --cov=mesa_geo tests/
>   from mesa_geo.visualization.ModularVisualization import ModularServer
E   ModuleNotFoundError: No module named 'mesa_geo.visualization.ModularVisualization'
examples/rainfall/rainfall/server.py:5: ModuleNotFoundError

Expected behavior
Running the test suite should not give any errors

Indeed I don't see the module anymore in the visualization folder of mesa-geo

Maybe I'm getting confused from #76

Or I simply messed up the dev installation?

@wang-boyu
Copy link
Member

Hmm this is interesting. But I do not have this issue on my side.

Could you try to use a clean virtual environment from scratch, and install dependencies and Mesa-Geo via pip install -e ".[dev]"?

@mrceresa
Copy link
Author

Hey, thanks for the quick answer. I'm testing it now on my home pc (ubuntu)

This is what I tried;

git clone https://github.com/projectmesa/mesa-geo.git
cd mesa-geo/
mamba create -n testmg python
mamba activate testmg
pip install -e ".[dev]"
pytest --cov=mesa_geo tests/
[...]
    import mesa
>   from mesa_geo.visualization.ModularVisualization import ModularServer
E   ModuleNotFoundError: No module named 'mesa_geo.visualization.ModularVisualization'

examples/geo_schelling/server.py:2: ModuleNotFoundError

Here the full output of the pytest errors and the mamba package list:
pytest_errors.txt
mamba_list.txt

Thanks for your help!

@wang-boyu
Copy link
Member

I see. Looks like Mesa-Geo is not properly installed for some reason.

We do not have a mesa_geo.visualization.ModularVisualization module in our code base; it is actually copied over from Mesa during setup. See:

def get_mesa_viz_files():

How about pip install -e . or install from PyPI directly, i.e., pip install mesa-geo? I am not very sure why this is happening.

@mrceresa
Copy link
Author

okay so this is interesting: it seems that (at least on my machine) there is a subtle difference in the way setuptools treats command classes between install and install_editable with pip.

I start from a fresh clone (so the ModularVisualization.py file is not there) then:

  1. If I install with pip install . the build command is properly called and the file is copied. Everything work and this is the expected behaviour
  2. This is also the case, if I manually run python setup.py install/develop
  3. Now, if I install with pip install -e . the develop command is not called and I don't get the visualization/ModularVisualization.py. Tests then obviously fail.
  4. This is also the case for pip install -e .[dev]

So the question is, why --editable is not calling neither the build nor the develop subcommands?

Actually pip install . calls the custom build command, as we can see from this log ("INSIDE BUILD") is my debug text

Building wheels for collected packages: Mesa-Geo
  Created temporary directory: /tmp/pip-wheel-uy4v8atq
  Destination directory: /tmp/pip-wheel-uy4v8atq
  Running command Building wheel for Mesa-Geo (pyproject.toml)
  /tmp/pip-build-env-6f4grtxt/overlay/lib/python3.10/site-packages/setuptools/config/setupcfg.py:508: SetuptoolsDeprecationWarning: The license_file parameter is deprecated, use license_files instead.
    warnings.warn(msg, warning_class)
  running bdist_wheel
  running build
  INSIDE BUILD
  Downloading the leaflet.js dependency from the internet...
  Downloading the leaflet.css dependency from the internet...
  running build_py

On the other side, pip install --editable . calls editable_wheel and not develop

Building wheels for collected packages: Mesa-Geo
  Created temporary directory: /tmp/pip-wheel-0upylsq5
  Destination directory: /tmp/pip-wheel-0upylsq5
  Running command Building editable for Mesa-Geo (pyproject.toml)
  /tmp/pip-build-env-atgt3u8t/overlay/lib/python3.10/site-packages/setuptools/config/setupcfg.py:508: SetuptoolsDeprecationWarning: The license_file parameter is deprecated, use license_files instead.
    warnings.warn(msg, warning_class)
  running editable_wheel
  creating /tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info
  writing /tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/PKG-INFO
  writing dependency_links to /tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/dependency_links.txt
  writing requirements to /tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/requires.txt
  writing top-level names to /tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/top_level.txt
  writing manifest file '/tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/SOURCES.txt'
  reading manifest file '/tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/SOURCES.txt'
  adding license file 'LICENSE'
  writing manifest file '/tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo.egg-info/SOURCES.txt'
  creating '/tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo-0.3.0.dist-info'
  creating /tmp/pip-wheel-0upylsq5/tmpjw6p7snr/Mesa_Geo-0.3.0.dist-info/WHEEL
  /tmp/pip-build-env-atgt3u8t/overlay/lib/python3.10/site-packages/setuptools/command/install.py:34: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
    warnings.warn(
  running build_py
  running egg_info

Thus no file is copied...

@mrceresa
Copy link
Author

As a proof, if I change the setup.py accordingly:

--- a/setup.py
+++ b/setup.py
@@ -12,6 +12,7 @@ from distutils.command.build import build
 
 from setuptools import setup
 from setuptools.command.develop import develop
+from setuptools.command.editable_wheel import editable_wheel
 
 
 def get_version_from_package() -> str:
@@ -22,10 +23,22 @@ def get_version_from_package() -> str:
     return version
 
 
+class EdiWheelCommand(editable_wheel):
+    """Installation for development mode."""
+
+    def run(self):
+        print("INSIDE EDIWHEEL")
+        get_mesa_viz_files()
+        get_frontend_dep()
+        editable_wheel.run(self)
 
 if __name__ == "__main__":
+
     setup(
         name="Mesa-Geo",
         version=get_version_from_package(),
         cmdclass={
+            "editable_wheel": EdiWheelCommand,
             "develop": DevelopCommand,
:


Building wheels for collected packages: Mesa-Geo
  Created temporary directory: /tmp/pip-wheel-556vurov
  Destination directory: /tmp/pip-wheel-556vurov
  Running command Building editable for Mesa-Geo (pyproject.toml)
  /tmp/pip-build-env-ysf_0t7s/overlay/lib/python3.10/site-packages/setuptools/config/setupcfg.py:508: SetuptoolsDeprecationWarning: The license_file parameter is deprecated, use license_files instead.
    warnings.warn(msg, warning_class)
  running editable_wheel
  INSIDE EDIWHEEL

And the ModularVisualization.py file is present again!

@rht
Copy link
Contributor

rht commented Sep 28, 2022

We should replace the get_mesa_viz_files mechanism. This pattern seems to be non-standard in Python package setups.

@mrceresa
Copy link
Author

Yes, if you can confirm this, we could:

  • Leave it like this (nobody a part me seem to have had the same problem)
  • Extend the build_py command instead of build and develop as this seems to be always called
  • Maybe consider using the recent sub_command extension from setuptools to add additional build steps

And additional question: is there any reason why build is imported from distutils and the rest from setuptools?

@wang-boyu
Copy link
Member

And additional question: is there any reason why build is imported from distutils and the rest from setuptools?

If I remember this correctly, I could only subclass build through distutils, not setuptools.

We should replace the get_mesa_viz_files mechanism. This pattern seems to be non-standard in Python package setups.

Agreed! Shall we make a quick fix before addressing #76, or work on #76 directly?

@wang-boyu wang-boyu added the bug Release notes label label Sep 29, 2022
@wang-boyu wang-boyu added this to the v0.4.0 milestone Sep 29, 2022
@wang-boyu
Copy link
Member

@mrceresa If get_mesa_viz_files() is not called as expected, how about get_frontend_dep()? Do the Leaflet files (js and css) get downloaded?

If not then we may still need to address this issue for the Leaflet dependency.

@zlfdodo
Copy link

zlfdodo commented Oct 5, 2022

I got the same problem when I installed mesa-geo from git. I reinstall this package by using 'pip install mesa-geo' and it works.

@wang-boyu
Copy link
Member

@zlfdodo Sorry about that. Could you share which operating system and python version were used?

@zlfdodo
Copy link

zlfdodo commented Oct 5, 2022

@wang-boyu Thanks for the prompt reply. I am using Windows system and conda python 3.10.6. The mesa-geo package only works under this python version on my computer. When I use any other python version in Anaconda and run any model/run.py file in the example folder, it will return "ImportError: DLL load failed while importing_version."

@wang-boyu
Copy link
Member

Yes, if you can confirm this, we could:

  • Leave it like this (nobody a part me seem to have had the same problem)
  • Extend the build_py command instead of build and develop as this seems to be always called
  • Maybe consider using the recent sub_command extension from setuptools to add additional build steps

I managed to reproduce this issue with Ubuntu Kinetic and Python 3.10 - the develop command is not called during editable install, which is really weird. But the build_py command does seem to work.

As of the sub_command method, looks like it's still an open issue, so I'll most likely go for the build_py approach.

I'm currently waiting for the Mesa 1.1.0 release to submit a PR for #76 which also involves changes to setup.py. Then I'll work on a separate PR for this issue.

@wang-boyu
Copy link
Member

Regarding Windows OS, I don't have access to any Windows PC at the moment. Let me see what I can do.

This may also link to #30.

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

Successfully merging a pull request may close this issue.

4 participants