search

brain-score / model-tools

Helper functions to extract model activations and translate from Machine Learning to Neuroscience
MIT License
8 stars 27 forks source link

Update setup.py with missing dependencies #29

Closed franzigeiger closed 4 years ago

mschrimpf commented 4 years ago

What's the rationale for moving these checks from brainscore to model-tools?

franzigeiger commented 4 years ago

They use model tool classes and I don't want to have a circular dependency between brain-score and model-tools

mschrimpf commented 4 years ago

My preference is to leave the framework dependencies out of the setup.py because including them would force all users to install all dependencies (while most only operate in one of them). Instead, we specified them in https://github.com/brain-score/model-tools/blob/2f778c62032073503bf9685448aa2d24dd1a8e20/.travis.yml#L25-L26 -- could we do something similar for Jenkins?

franzigeiger commented 4 years ago

Well the setup.py is the file where one should list all requirements the project has. Not listing them basically destroys the purpose of the requirements section. If sb doesn't want to install all dependencies they can do that by hand but that's definitely not the use case we should optimize for. I personally also much rather install a bunch of not instantly required libraries to prevent things from failing. For jenkins this is an issue as well because the script is not customized per project and I would like to keep it that way. Meaning the projects have to be installable and ready to go only based on setup.py. The same goes for all other projects. I'm adding all required libraries in the setup.py's.

mschrimpf commented 4 years ago

I'm worried about the failing unit tests here, do they not belong to this PR? Current master seems fine, does this merge make it unstable?

franzigeiger commented 4 years ago

Well the master state is only Travis, which is also green here. Those six failing tests also failed before afaik.