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

Update for PsN 4.7.0, extend build to include threads and updating the NONMEM license #6

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@billdenney
Copy link
Collaborator

commented Jun 22, 2017

This pull addresses #2, #3 (for PsN but not for NONMEM), #4, and #5.

Specifically:

  • It updates to PsN 4.7.0 (also adding the YAML::Tiny module which is suggested by 4.7.0)
  • It updates the way that PsN interfaces with NONMEM so that it is no longer version dependent (looking in /opt/nm). This fails PsN autodetection of NONMEM installations, so the directory name has been provided in the expect scripts.
  • It makes the expect scripts more specific and restrictive (this should be a good thing because accidental failures will be more quickly noticed). In addition, it adds a failure to the scripts on timeout rather than a silent timeout.
  • It updates the default number of threads for PsN to 4 (aligning with 1 license pack from Icon), and makes the default configurable by the user.
  • It updates the build tagging to add "osmosisfoundation" and the PsN version number
  • It updates psnshell.sh to run as the current user (so that files created belong to that user on exit) and point to the updated directory structure and docker container name.
  • It requires the PsN tests to run successfully for the container to be generated (more on this below)

For requiring the tests, this may be controversial. The main limiting factor is the NONMEM license being up to date. Without a license, the tests will fail and the container won't be created. It may be worth adding a "NO_TEST" option, but my preference is that any container should always be tested before being created so that invalid containers can't exist.

@dpastoor

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

very nice - is this still in progress or is this ready to merge? If so I'll give it a shot myself as well, though I trust you :-) then merge

@billdenney

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

It's ready for merge (aka works for me). It's definitely better for you to give it a shot first.

@dpastoor

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

We do have one logistical issue - all this images are being autobuilt on docker hub - namely this means that no license file exists. This is also an issue as for people pulling the container, we can't have an embedded nonmem license that they can use. In theory, we could use a multistage build to build an underlying image, confirm that everything ran, then remove the license file again before publishing.

I spoke to Bob about this at PAGE (getting a FOSS 'build' license) and basically his response was "as long as you can demonstrate that there is no way other people could accidentally get their hands on it, we can talk".

Given that you have the licensestring - I think it could be set as an environment variable, maybe, though I dont' know if environment variables have a max string length off the top of my head.

@billdenney

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

Setting an environmental variable would work. For a FOSS license, we could do something like:

  • If NONMEM_LICENSE_STRING is given, use it as normal.
  • If FOSS_NONMEM_LICENSE_STRING is set to "Y" (or anything non-empty), then do a web request to the localhost where the license file is setup as the only available file (perhaps via Python's simplehttpserver), run the tests, and delete the license file all within the same step.

With deleting the file in the same step, it would never be available in the package (we couldn't do it with an environmental variable because the environmental variable would be available in the environment of either the final container itself or an intermediate container).

The only question there to me is: Can we host it in a way that it is available to the local build but not available to the world? That's mostly a question of the way that building containers works on Dockerhub.

@dpastoor

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

The problem is docker hub. I actually looked and it seems there isn't a way to set an environment variable.

moby/moby#6822

I meant setting on the actual host system, not in the container - like on travis/circleci/etc you can specify env variables to store secrets.

I think we're going to have to table the strictness for the public image, given that it essentially has to be completely public, and I can't think of any way of something being completely public, yet hiding the license info.

The solution I am thinking down the line, if people do want 'validated' images to pull from, is we could set up a private registry and charge some extremely nominal fee (basically to cover the cost of a private CI server) which could then build, validate, and host images. I'm doing that with a project right now on https://cloud.google.com/container-engine/ to host some images with private stuff like my ssh keys/licenses/etc so I can immediately spin those images up. The 'problem' is you have to pay for bandwidth egress and storage. It's quite a small $ but I'm hesitant to 'fracture' the flow for now, and would rather provide, for example, a mechanism to run a validation check on the persons system when they first provide a license file to do the double check that everything is running properly. It doesn't address your (valid) proposal that any image published we should be confident of its validity regarding a completely working install, but it keeps things open and useable.

@dpastoor

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

Sorry posted the container engine - I meant to post https://cloud.google.com/container-registry/

@billdenney

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

I fully agree that we should not have a fractured development environment.

Given this, I can rework the pull request so that if the license is given, the installation is tested and the testing scripts are removed. If the license is not given, the installation won't be tested, and the testing scripts will be kept (so that the user can run the tests later). Does that sound reasonable?

Edit: If the user gives the license file to the original NONMEM build, but doesn't give it to the PsN build, the PsN build won't be tested even though it theoretically could be tested. I think that's OK to require the license string to be given both times, but it may lead to some (hopefully minimal) confusion.

@billdenney

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 22, 2017

@dpastoor I've now tested with and without the license key, and it appears to work as intended. Can you run a few tests on your system and if it works for you, merge it?

@dpastoor

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2017

yep thanks for the changes - I'll run it through in a couple hours and confirm

@BretFisher

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2017

For most of the bullets in this PR, please see my comments in their respective issues so each concern can be discussed easier. I had some thoughts on #3 #4 and #5.

On the new feature of build tagging, the reason I didn't tag "local builds" with the project name, is that's the name of images being pulled from Docker Hub. It's s standard of sorts to ensure users can hold images from Hub in their cache of the "officially built" ones and still (optionally) build custom ones. If they are both the same image repo/tag then they'll overwrite each other.

I was downplaying build scripts in lue of docker-compose, as that's the purpose of it, to prevent build/run scripts around the docker cli. That build.sh is still there and maybe not needed (some cleanup from before I started is likely necessary). One of my design goals for this repo was to end those scripts when possible, and focus on the many features of docker-compose for local image and container management. docker-compose will build w/o needing those scripts AFAIK, and if we need to wrap anything to make it easier for the user, we should wrap docker-compose IMHO.

@billdenney

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2017

Everything you've noted sounds good. I've not done docker-compose work before, so I'll be interested to see how it works. This user would vote for a wrapping of docker-compose to simplify the local build option.

@BretFisher

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2017

Yea one thing that this whole repo is light on is user workflow documentation, largely due to me needing @vjd to tell me that workflow 😄

My goal is:

git clone xxxx
cd pmx-docker
docker-compose up

It starts all images with defaults, which are changeable either on cli or by editing yml.

Then they can docker-compose exec nonmem command

docker-compose is designed to be a local workflow tool to make using docker cli easier by storing most of your options in yml, allowing overrides, auto naming everything, etc.

@vjd

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

can I get a clone of myself, please? I promise July 10 is pretty much the end of all my major projects for this summer, and not taking on anymore. Dedicated time to osmosis stuff!

@BretFisher

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2017

It would be great (a pain I know) to split these out to be one-PR-per-GHIssue. I'm working the issues one-by-one now so might put some of your code here into PR's if I get to it first.

@BretFisher

This comment has been minimized.

Copy link
Collaborator

commented Jul 19, 2017

Hey @billdenney, not wanting to steal your work or thunder on this, but thought it easier to break each thing up into a separate effort, that I worked on adding this week. Based on my master commits this week, this is where we're at from your original PR change list. My comments in bold.

  • It updates to PsN 4.7.0 (also adding the YAML::Tiny module which is suggested by 4.7.0)
  • It updates the way that PsN interfaces with NONMEM so that it is no longer version dependent (looking in /opt/nm). This fails PsN autodetection of NONMEM installations, so the directory name has been provided in the expect scripts.
  • It makes the expect scripts more specific and restrictive (this should be a good thing because accidental failures will be more quickly noticed). In addition, it adds a failure to the scripts on timeout rather than a silent timeout.
  • It updates the default number of threads for PsN to 4 (aligning with 1 license pack from Icon), and makes the default configurable by the user.
  • It updates the build tagging to add "osmosisfoundation" and the PsN version number (version numbers added in Docker Cloud builds. Local builds with docker-compose-build.yml use latest)
  • (didn't add) It updates psnshell.sh to run as the current user (so that files created belong to that user on exit) and point to the updated directory structure and docker container name. this works without this modification when I test on Mac/Windows so far. If we have a permissions problem can you open a new issue @billdenney with the details, thanks
  • (didn't add) It requires the PsN tests to run successfully for the container to be generated I think we should wait for multi-stage support before adding nonmem license to builds. We can track/discuss in #4

If you're cool with all that, could we close this PR @billdenney

@billdenney

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 19, 2017

No worries, and I don't feel it's stealing my thunder.(Though I may send a PR correcting my last name and business name in the Readme file.) :)

I'm cool with the changes. The two "didn't add"s I'd like to have, but I'll open new issues to cover those specifically.

@billdenney billdenney closed this Jul 19, 2017

@billdenney billdenney referenced this pull request Jul 19, 2017

Open

PsN Tests Are Not Run #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.