build: move installation of task configs to ch_ansible

anjakefala commented 4 years ago

we previously used --install-options when installing dias
bc of this pip bug https://github.com/pypa/pip/issues/1883, we cannot use that option in the same install command
sister PR https://github.com/chime-experiment/ch_ansible/pull/91

anjakefala commented 4 years ago

The build is failing because the CI server does not have a key to read the private repos with. This results in trouble installing requirements.txt, and then dias does not start up.

I am going to try to figure out how to fix this after the f2f.

jrs65 commented 4 years ago

Okay. I appreciate, I'm not being asked to review this, but I'm getting notifications for some reason so I couldn't help but look.

I think there must be a better way to tackle this one, it seems like removing the install_requires options entirely is contrary to the way most python packaging works.

It looks like thing that you are doing in all the code around the task-conf would be much more appropriately done via ansible, and then you could remove the custom option and put the install_requires back in.

nritsche commented 4 years ago

The dias CI server should use the ch_util stub listed in docs/requirements.txt. It looks like it installs that but then also wants to get the real ch_util because it's a requirement for draco...

jrs65 commented 4 years ago

ch_util isn't a requirement for draco.

anjakefala commented 4 years ago

@jrs65, can you please explain what you mean by this?

It looks like thing that you are doing in all the code around the task-conf would be much more appropriately done via ansible, and then you could remove the custom option and put the install_requires back in.

--task-conf is needed as a pre-requisite to DIAS installing. It currently assumes specifying via the commandline. Do you mean that you would like us to change it to an environment variable?

is it even going to ever be anything but /etc/dias.conf?

anjakefala commented 4 years ago

@ketiltrout

Do these changes require ch_ansible changes, too?

Yes, I had opened a separate PR: https://github.com/chime-experiment/ch_ansible/pull/91

I would not bother reviewing it, until we finish discussing things here.

anjakefala commented 4 years ago

The dias CI server should use the ch_util stub listed in docs/requirements.txt. It looks like it installs that but then also wants to get the real ch_util because it's a requirement for draco...

@nritsche

Yeah, draco has it as a requirement.... I will check if draco can be imported without having a real ch_util.

Does that mean that docs/requirements.txt is specifically for CI server requirements?

jrs65 commented 4 years ago

@jrs65, can you please explain what you mean by this?

It looks like thing that you are doing in all the code around the task-conf would be much more appropriately done via ansible, and then you could remove the custom option and put the install_requires back in.

--task-conf is needed as a pre-requisite to DIAS installing. It currently assumes specifying via the commandline. Do you mean that you would like us to change it to an environment variable?

is it even going to ever be anything but /etc/dias.conf?

This last point is a big part of the question. Why is --task-conf even an option if you always install to the same place? Doing that for a Python package is very non-standard as evidenced by the fact it breaks everything else. And you shouldn't have to remove the install_requires to make it work, that's just another indicator it's the wrong approach.

I think you should remove the task config option and either:

Always install the task configs to /etc/dias in setup.py if doing a system install
Use ansible to install the task config files, and don't install anything in setup.py. This one is probably more preferrable and is the pattern we use pretty much everywhere else (coco, comet, kotekan, ...). Plus it fits in better with what you already do in ch_ansible: install the dias.conf; moving around the old/new task directories

Whatever you do, definitely don't remove install_requires! Particularly as you were thinking about how to make software installations more robust, removing the at install time dependency checking is likely to make things worse.

jrs65 commented 4 years ago

The dias CI server should use the ch_util stub listed in docs/requirements.txt. It looks like it installs that but then also wants to get the real ch_util because it's a requirement for draco...

@nritsche

Yeah, draco has it as a requirement.... I will check if draco can be imported without having a real ch_util.

Does that mean that docs/requirements.txt is specifically for CI server requirements?

@anjakefala @nritsche I keep saying this, but draco really doesn't have ch_util as a requirement. That's its raison d'etre, it's all the pipeline code which doesn't depend on CHIME packages.

anjakefala commented 4 years ago

@jrs65 @nritsche @ketiltrout I updated this PR and ch_ansible. They are ready for review, as a pair.

There is a link to the ansible PR in the updated description of this PR.

chime-experiment / dias

build: move installation of task configs to ch_ansible #152