lmfit should be a dependency?

[eteq]

It seems like a lot of the omnifit functionality requires lmfit. Should it be a requirement for installation?

[RiceMunk]

Sorry for the late reply; been forgetting to pay attention to the issue tracker.

Yes, lmfit should be a requirement. I'll be adding it to the requirements list.

[RiceMunk]

I think #18 should deal with this as far as automatically recording dependencies on pip is concerned. As per the recommendation of the astropy setuptools scripts I'm not adding the requirements to the setup.py script.

Will do so later if it turns out that I misunderstood how to sort out dependency resolution.

[eteq]

@RiceMunk - I don't quite understand why you closed this: adding a pip-requirements doesn't actually change anything about how dependencies are resolved. For example, if I just do pip install omnifit right now it does not install lmfit, which is what you want if lmfit is supposed to be a dependency.

Or are you saying you think lmfit should not be a hard dependency, but rather an "optional" dependency?

[RiceMunk]

Okay, it looks like I've misunderstood how pip resolves dependencies and as a result prematurely closed this issue. Apologies; this is the first time I've been dealing with pip dependency resolution and I seem to have misunderstood the guides I looked at. lmfit is definitely a hard dependency, for now.

Reopening this now, but I suppose it's useful to try and explain the logic I used earlier:

Initially I was under the impression that I should go and add lmfit (and the other required packages, as listed in the documentation) to the install_requires key of setup.py. However, when I was about to do that, I bumped into the following bit of commentary in the astropy-provided setup.py:

# Note that requires and provides should not be included in the call to
# ``setup``, since these are now deprecated. See this link for more details:
# https://groups.google.com/forum/#!topic/astropy-dev/urYO8ckB2uM

Because of this I decided against messing around with the relevant part of setup.py and, after looking at a few more established astropy-affiliated packages with extra requirements, found the mention of a pip-requirements file. I figured that this is the way it should be done with astropy-related packages and went with that instead. Of course this ended up not being automatically parsed by pip, as I thought it would, but I did go an add the relevant packages to the requirements list on pypi. But I guess that doesn't do what I expected either?

So yeah, I went and messed up here. I guess I should have poked around a bit more before I jumped the gun.

I'm currently tempted to just update the install_requires key appropriately, and see if that works. I'll probably do so sometime next week, when I have free time to dedicate to testing that I'm doing it properly this time. In the meantime though, comments are very welcome on what is the actually correct way of doing this.

[eteq]

@RiceMunk - Aha, gotcha, I see how you reached that conclusion. So what it's indicating is actually that requires and provides are deprecated, in favor of install_requires/setup_requires. So indeed, I think all you need to do is add install_requires=['lmfit', 'astropy>=1.0', ...] (or whatever your actual requirements are).

We'd welcome a PR to the package-template to fix up that comment to explain better what the situation is. You're probably best suited to doing that simply because you understand what it is that lead you to the conclusion you reached, and therefore hopefully how to avoid it!

[RiceMunk]

Alright, that makes sense. I suppose I was a bit overzealous with my interpretation of the comment in setup.py, then. I'll see if I can come up with an alternate way of formulating the comment and put a related pull request on package-template.

Anyhow, there's a 0.2.1rc1 on PyPi right now which does seem to properly parse the requirements now, and install new stuff as necessary.

This has, however, brought up a strange issue (#22) where the fitting is now converging in silly places when I try to run a simple test fit with a newly installed version of Omnifit on a test VM. I'm suspecting this may be related to some new version issues on one/some of the dependencies, so I'm leaving this issue open for now, as it may be related.

[RiceMunk]

22 was indeed sort of related, as it was a problem with how lmfit behaviour had changed in a recent update. See that issue and its related commit for more details.

Closing this, since it seems to be sorted for good now.