Skip to content

Conversation

@rht
Copy link
Contributor

@rht rht commented Jan 3, 2019

  1. click: only used in tests, hence should be put in dev
  2. jupyter: only appears in examples
  3. networkx: only appears in test_space.py and examples (technically it is implicitly required in NetworkGrid, but again, not every users have to install networkx if they don't use NetworkGrid at all)

This should minimize the install footprint of any libraries that depend on Mesa.

@rht
Copy link
Contributor Author

rht commented Jan 3, 2019

pandas is a heavy dependency. If only there is a way to construct a DF without having to pull in the full-blown library.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 79.794% when pulling a225918 on rht:reqs into 6e40c76 on projectmesa:master.

@coveralls
Copy link

coveralls commented Jan 3, 2019

Coverage Status

Coverage remained the same at 79.794% when pulling 4df21f1 on rht:reqs into 236edca on projectmesa:master.

@Corvince
Copy link
Contributor

Corvince commented Jan 8, 2019

click is required for the "mesa" command, so shouldn't be removed
jupyter is a very good package to remove from the dependencies !!
networkx provides functionality that one would very much expect from any ABM package. I would strongly favor to keep it as a dependency (and its not that heavy either)
pandas is probably used by 95% of people using mesa, so I don't see the need to remove it as a dependency (and have people drip over the missing requirement)

It is true that we could remove networkx and pandas, but judging from the issues and the mailing list a lot of mesa users are python beginners. That's way I think of mesa as a "batteries included" package that should ship with everything needed. While it is really not that difficult to install missing requirements, I wouldn't want to risk losing people. Networkx is not that big and pandas is so widely used.

That said it would be nice to have the ability to only install "core" mesa, i.e. without cookiecutter , click and pandas. Could be useful for web services. But I don't know how we could provide this

@rht
Copy link
Contributor Author

rht commented Jan 8, 2019

click is required for the "mesa" command, so shouldn't be removed

Ah, didn't see the import click at main.py. I have only grepped from click import.

My specific use case is for an ABM library that is built on top of Mesa. Having lots of those huge libraries slows down CI test considerably, especially the ones where they have to be cython-compiled even if the pip download has been cached. Also, indirect dependencies sometimes causes install gotchas.

General arguments on whether to include examples dependencies into library dependencies:

It is true that we could remove networkx and pandas, but judging from the issues and the mailing list a lot of mesa users are python beginners. That's way I think of mesa as a "batteries included" package that should ship with everything needed.

Every examples directory already has requirements.txt. Error messages caused by missing imports are explicit. The requirements of the examples (they will keep increasing) shouldn't be included in the library's requirements. What if there is an example that uses sklearn/tensorflow/pytorch/any-other-huge-library?
One middle ground is to add extras.txt (how networkx does it https://github.com/networkx/networkx/tree/master/requirements) and so people can install via pip install mesa[extras].

Orthogonal to the rest:

pandas is probably used by 95% of people using mesa, so I don't see the need to remove it as a dependency (and have people drip over the missing requirement)

That's an assumption along the line that 0.95 * everyone uses matplotlib and the 5% uses seaborn/bokeh/altair/...
(I use pyarrow myself because pandas is bloated)

@rht
Copy link
Contributor Author

rht commented Jan 8, 2019

Note: networkx import is 6x longer than scipy import, which means it is heavy.

python3 -X importtime -c 'import networkx' 2> out.log
tuna out.log

2019-01-08-233102_1870x845_scrot

@Corvince
Copy link
Contributor

Corvince commented Jan 9, 2019

My comment wasn't meant to be applicable to example requirements, but just networkx. Sorry if you got that impression. And so far networkx only gets imported if you use it, so I don't see how the import time is relevant here.

But I am very positive that #617 will get merged and then networkx will be part of the main library anyway (since, as I said, networks are a key ABM feature).

Regarding pandas I would like to get some more comments by the other developers, but I am not against removing that dependency. Sorry if my comment was misleading but I just don't think we need to remove it, but yes, it could be removed.

For click/cookiecutter it would also be an idea to just have the mesa commands as an [cli] install (if that is possible?) or even a separate repo. I don't like the current way of simply adding a CLI without asking or even notifying the user.

@jackiekazil jackiekazil self-assigned this Jan 11, 2019
@jackiekazil
Copy link
Member

jackiekazil commented Jan 18, 2019

As the PR stands with removing Networkx and Jupyter....
Reading through this all... I see both sides of networkx in or out. I am torn.
@dmasad and I discussed it the other day during the dev meeting and thought that maybe a compromise is to take it out, but put it in for test builds and if someone tries to do create a network from scratch that an error would explicitly tell them to install Networkx. While the error would naturally show a failed import, not everyone building models is going to know what that means.

@rht Ty for including the log output. That is interesting.

I think a minor adjustments would need to happen before a merge.

  1. Why include networkx in the dev requirements? (Is that just to throw in the kitchen sink?) I am okay with there. I just wanted you to really think about it.
  2. Add it to the test build, because of adds wrappers for networkx so you can modify the graph from within mesa #617 and then I think that would cover that.
  3. Can you update your branch b/c of the broken build? (I think it might just be a update to the branch, because of set pytest version >=3.6 #622.)

Optional -- #4 - create an explicit error message that tells people to install networkx. (Or we can create a ticket and I or someone else can do it.)

@jackiekazil
Copy link
Member

RE: Pandas -- maybe we should move that to a discussion ticket? Maybe there is a way to create an alternate option and you can override Pandas?

@rht
Copy link
Contributor Author

rht commented Jan 18, 2019

Why include networkx in the dev requirements? (Is that just to throw in the kitchen sink?) I am okay with there. I just wanted you to really think about it.

Because it is used in tests, nx.complete_graph().

Add it to the test build, because of #617 and then I think that would cover that.
While the error would naturally show a failed import, not everyone building models is going to know what that means.

#617 makes the networkx dependency explicit.
Did you mean:

  1. Do not include networkx in requirements, then
  2. Do lazy-import of networkx at the initialization of NetworkGrid, if import fails, then print out a message to install networkx instead of printing out all the error stack
    -- once again, this is more of to decide whether Mesa should be a kitchen sink library-suite or a base library for more specialized ABM libraries to build on top of.

@Corvince
Copy link
Contributor

-- once again, this is more of to decide whether Mesa should be a kitchen sink library-suite or a base library for more specialized ABM libraries to build on top of.

I guess there are arguments for both ways. But if we want to have a specialized library, I think we can remove all dependencies, no?

  • click, cookiecutter only used for mesa command
  • numpy only used for ContinuousSpace
  • pandas see discussion above
  • tornado only used for Visualization
  • tqdm only used for batch runner

Given the ease of installing mesa with pip install mesa[extras] (or similar name), I don't see a good reason not to remove all dependencies. But this should probably be discussed more broadly.

@rht
Can you change this PR only to remove jupyter (which seems fine for everyone)? And could you have a look at the documentation if it assumes the presence of jupyter and change it to give install instructions if yes? Then we could merge this for now without further ado.

I will open an issue about removing all dependencies and the steps necessary for that.

@Corvince Corvince mentioned this pull request Jan 21, 2019
3 tasks
@rht
Copy link
Contributor Author

rht commented Jan 21, 2019

Amended so that it removes jupyter only. All examples have jupyter in their requirements.txt files.

@Corvince Corvince merged commit b7b350b into projectmesa:master Jan 21, 2019
@jackiekazil
Copy link
Member

This kind of feels like the Django versus Flask approach. Both are valid. I am wondering if we can be both?

Anyways... @rht for starting this & @Corvince for kicking off discussion.

@jackiekazil jackiekazil added this to the Lake Havasu City v0.8.6 milestone Jan 22, 2019
@Corvince Corvince mentioned this pull request Feb 4, 2019
@rht rht deleted the reqs branch September 16, 2023 08:27
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.

4 participants