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

Bulk changes to the recipes and build order. #7

Merged
merged 4 commits into from
Jul 3, 2018

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Jun 30, 2018

Bulk changes to the recipes and build order.

Fixes #3
Fixes #4

@jeanconn
Copy link
Contributor Author

I started with individual commits and then it just got silly.

@jeanconn
Copy link
Contributor Author

For a few of builds, I installed with pip instead of python setup.py install because it made the module tests work. If we don't care about the tests, I'm not sure if we care about the install, but I suspect this has something to do with egg dirs or whatnot and we should probably check that the built packages match what is somewhat expected to traverse them with testr.

@taldcroft
Copy link
Member

I had assumed that using pip install per the spec in python_modules.cfg was the always the way to go. But I don't know if there are any unexpected interactions with conda.

@taldcroft taldcroft mentioned this pull request Jun 30, 2018
@jeanconn
Copy link
Contributor Author

And yes, for the few that failed I use the pip install per the python_modules.cfg spec. I think we can change them all, we'd probably just need to respect --egg. And yes, concern about interaction with the meta.yaml preserve_egg_dirs is why I didn't change this in bulk, though it should be easy to rebuild and retest if that's the way we end up going.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits. Overall this is amazing!!!

# Packages required to run the package. These are the dependencies that
# will be installed automatically whenever the package is installed.
run:
- ska.shell ==3.3.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize for convenience in finding a package to update version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We may end up doing this in other packages too, as largely they just accumulated dependencies as they were considered.

- setuptools
- matplotlib
- numpy
- pytest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general seems this should be testr, which is directly imported in the package and then requires pytest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the mica test packages import pytest directly so I think it is fine to have them both (testr and pytest) even if you end up with redundancy in the dependency tree.

- matplotlib
- numpy
- pytest
- django
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a bunch of packages, we have imports that are only used in certain circumstances. For mica, django is needed for mica.web. Obviously mica.web is useless unless we're running kadi webserver, in which case there is already a django.

I agree it is probably fine to remove django from here.

run:
- python
- six
- setuptools
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setuptools should not be an explicit runtime requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check.

- six
- scipy
- numpy
- testr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testr is required to do import maude; maude.test().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth on this and can obviously go back again. It did seem to me that testr is more of a test dependence than a runtime dependence, and in a real ska runtime environment, there's always going to be a testr... but if we want to make sure that each ska package is good about also pulling in testr from the dependency tree.. that's fine too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to the trouble of enumerating Ska package dependencies at all, we should make them complete and not depend on being in a "real" Ska runtime. Otherwise we could pretty much skip all the meta.yaml run requirements (except for build-time testing).

Copy link
Contributor Author

@jeanconn jeanconn Jul 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll move more of the test dependencies into the runtime area (without getting crazy).

@@ -29,7 +28,8 @@ requirements:
- pytables
- configobj
- requests
- django
- tables3_api
- django <2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice way to encapsulate this locally!

cxotime
annie
ska-flight
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface doc specifies ska3-flight. I went back and forth about whether to try being consistent with ska3 branding instead of ska to make this distinction clear to ourselves and end-users. In a few years the 3 might seem superfluous, but for now when we say "Ska flight" that is really Ska2 flight, so probably worth being rigorous in here.

- quaternion
- chandra.time
- ska.sun

test:
requires:
- agasc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a case where we have some involved testing that seems to need agasc and Ska.engarchive. I suppose it would be fine to just add those as runtime dependencies, but it seemed overkill (they don't really seem like run-time requirements for the package even if they are needed to complete all of its tests).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting web we are weaving. Agreed this is fine to leave as is.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably makes sense to just merge this now?

@jeanconn
Copy link
Contributor Author

jeanconn commented Jul 3, 2018

I've got some more 'misc' type stuff and that setuptools tweak and such that can probably go in here , but if you are waiting on this to do something, I can just put them in a new PR.

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.

2 participants