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

WIP Shiny packages updates (CentOS6 conda 4.7 so far) #230

Closed
wants to merge 20 commits into from
Closed

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Nov 28, 2019

Recipe changes to start to build modern packages with conda 4.7.

This was just building and testing on CentOS6 .

@jeanconn jeanconn changed the base branch from master to windows November 28, 2019 03:44
@@ -1 +1 @@
pip install --no-deps --verbose --no-binary :all: --no-index --egg .
python setup.py install --old-and-unmanageable
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 assume we could also just set a standard build in a environment variable.

@taldcroft
Copy link
Member

I had imagined following the lead of #227 with simply: script: python setup.py install in the build: section of meta.yaml and get rid of build.sh. This relies on conda to take away the egg dir by default, which is fine, and has the advantage that we will be already set up for Windows builds. When we migrate all the namespace packages to take out the Ska/__init__.py files, then we can get rid of the preserve_egg_dir directives, and achieve minimal meta.yaml files.

@jeanconn
Copy link
Contributor Author

Sure, it was just easier to batch update the build.sh files for a first go, and as far as I could tell there were a few different common 'python setup.py install' idioms to speed it up for conda use.

@@ -18,7 +18,6 @@ requirements:
# Packages required to build the package. python and numpy must be
# listed explicitly if they are required.
build:
- pip <10.0
- python
- setuptools
Copy link
Member

Choose a reason for hiding this comment

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

While you are at it I propose to move all the explicit setuptools dependencies. Yes, they are a dependence, but this is always provided by python so this is superfluous. There are a number of other packages also provided by conda python without which everything will break, but we don't explicitly specify those.

@@ -41,7 +40,7 @@ requirements:
- ska.engarchive
- mica
- testr
- llvmlite ==0.18.0

Copy link
Member

Choose a reason for hiding this comment

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

Extra blank line.

@@ -30,7 +30,7 @@ requirements:
- configobj
- requests
- tables3_api
- django <2.0
- django
Copy link
Member

Choose a reason for hiding this comment

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

Letting django float (to 2.2) requires sot/kadi#143 and a re-build of events.db3. I think we can and should go in that direction, but commenting that I believe this won't work out of the box.

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 thought the modern django was actually a requirement for modern dependencies to work, so I was building kadi out of that branch to start.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if any of the rest of our OTS stack depends on django, but who knows. In any case I'm not arguing against the latest django, just giving a heads-up that it will break kadi hard without a database migration.

Copy link
Member

Choose a reason for hiding this comment

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

Actually it's worse than just the database, IIRC there are changes that won't allow kadi to import.

ska_builder.py Outdated Show resolved Hide resolved
@@ -1,2 +1,2 @@
export SKA=/dev/null
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead just ensure that the SKA is set in ska_builder.py i.e. if currently undefined then set to /proj/sot/ska. Currently cheta is strict and won't import without finding an archive, while mica will import as long as SKA is defined to something. This is somewhat a philosophical discussion about whether a package should import if it does not have the data to actually function, and perhaps situations like this show the way (contrary to my past thoughts).

The right answer from the packaging perspective is moving to single-source version so setup.py can run without importing the package, but in the meantime I think we can clean up these recipes assuming integration with ska_builder.py (which is already the case with the SKA_TOP_SRC_DIR).

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 was a little concerned, since our default build will likely be on a Linux machine that will see the real /proj/sot/ska and run as the aca user, that giving any more recipes access to /proj/sot/ska increased risk.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough to not change the ska_builder.py script right now, instead just use script env SKA=whatever python setup.py install.

Nope, that won't work on Windows. So back to setting SKA to something in ska_builder.py. Honestly I'm not that concerned about python setup.py install ever writing to something in SKA if it exists. What happens there is importing various packages (primarily to get the version), and I always consider it safe to import any Ska package as aca. If the act of importing a package can corrupt our flight database then we have bigger problems.

Did you have some specific failure path in mind that I'm missing?

Nevertheless we can be super careful just the same in ska_builder.py by making a temp dir, point SKA at that, and then make a dir SKA/eng_archive/data. That will satisfy cheta and mica to get them to import.

@taldcroft
Copy link
Member

Indeed changing to use python setup.py install in meta.yaml will require writing a little script, but it shouldn't be too bad. I think it's just looking for the first blank line after build: and inserting ' script: python setup.py install'. Good news is that you don't distinguish between namespace packages since that is already done everywhere with the preserve_egg_dir (as needed).

Is this script_env: - SKA_TOP_SRC_DIR still needed anywhere? Even now I don't see any build scripts that actually use it.

@@ -23,7 +23,6 @@ requirements:
- chandra_aca
- mica
- testr
- llvmlite ==0.18.0
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@@ -17,10 +15,12 @@ requirements:
# Packages required to build the package. python and numpy must be
# listed explicitly if they are required.
build:
- {{ compiler("cxx") }} # [ unix ]
host:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though now that I've fiddled with this some more, it will need Windows testing.

@taldcroft
Copy link
Member

When you say "is there a backstory here" I'm not sure what you mean? I was just trying to use the right options with the modern conda-build to get the conda compilers, as I think that is the "right way" and gives us more robust packages.

What I am looking for is a link or comment (documented in the recipe), which communicates why you think these are the right and necessary options. That will help me today and possibly your future self in 5 years.

@jeanconn
Copy link
Contributor Author

jeanconn commented Dec 3, 2019

Gotcha. Sounds good. Still fiddling with the new compile directive.

@@ -1,5 +1,6 @@
#!/bin/bash

mkdir ${PREFIX}/bin
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 should probably be mkdir -p on unix. Do we need ska3-template or a skare launcher on windows?

Copy link
Member

Choose a reason for hiding this comment

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

Not to my knowledge. If we find a need then we'll address it.

With regards to the -p, hasn't this been working? Don't we assume from the build process that ${PREFIX} must exist already?

Copy link
Contributor Author

@jeanconn jeanconn Dec 18, 2019

Choose a reason for hiding this comment

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

The "mkdir" fix is here because this recipe wasn't working for me on conda 4.7 . ${PREFIX} appeared to exist, but 'bin' did not in that case I think. I was just thinking "-p" might be more future-proof but probably don't have to worry about that

Copy link
Member

Choose a reason for hiding this comment

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

I am just worried that mkdir -p can make any arbitrarily path and do unexpected things. If $PREFIX exists then mkdir $PREFIX/bin should never fail, no?

@jeanconn jeanconn changed the base branch from windows to shiny December 18, 2019 19:09
@jeanconn
Copy link
Contributor Author

I'll pull out the change to ska_builder.py into a new PR .

@jeanconn jeanconn changed the title WIP Shiny WIP Shiny packages updates (CentOS6 conda 4.7 so far) Dec 18, 2019
@jeanconn
Copy link
Contributor Author

jeanconn commented Feb 3, 2020

I assume that the ca2ac95 changes are now superseded by #291

@jeanconn jeanconn closed this Feb 3, 2020
@taldcroft taldcroft deleted the shiny_v1 branch June 30, 2020 20:43
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