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.

Sign up for GitHub

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

Jump to bottom

Check python version against jar version #4328

Merged
merged 5 commits into from
Sep 19, 2018
Merged

Check python version against jar version #4328

merged 5 commits into from
Sep 19, 2018

Conversation

tpoterba
Copy link
Contributor

No description provided.

cseed
cseed previously approved these changes Sep 14, 2018
View reviewed changes
Copy link
Collaborator

@cseed cseed left a comment

Choose a reason for hiding this comment

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

I think we can clean this up once we build an installation package that includes the jar, python, C++ compiler and dependencies, etc. It will be much harder to pick up incompatible versions when everything is packaged together.

@tpoterba
Copy link
Contributor Author

I have no idea why the cloudtools piece is failing. am investigating

@tpoterba
Copy link
Contributor Author

AHHHH! It's because things work differently in a zip. Will have a think about this...

@tpoterba tpoterba dismissed cseed’s stale review September 17, 2018 18:49

dismissing because I had to redesign

@tpoterba
Copy link
Contributor Author

@cseed took a different approach, generating a python file which I import in hail initialization. This seemed like the only way to make things work in all the ways Python can be deployed (zip, egg, directory)

cseed
cseed reviewed Sep 19, 2018
View reviewed changes
hail/generate-build-info.sh Outdated
@@ -15,3 +16,4 @@ echo_build_properties() {
mkdir -p src/main/resources/

echo_build_properties $1 $2 > "src/main/resources/build-info.properties"
python scripts/parse_version_info.py src/main/resources/build-info.properties python/hail/_generated_version_info.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just create here python/hail/_generated_version_info.py the same way we do with build-info.properties?

cseed
cseed previously requested changes Sep 19, 2018
View reviewed changes
Copy link
Collaborator

@cseed cseed left a comment

Choose a reason for hiding this comment

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

See my other comment (meant to request changes! Go scorecard!)

@tpoterba
Copy link
Contributor Author

yes, that's better. done.

cseed
cseed reviewed Sep 19, 2018
View reviewed changes
hail/generate-build-info.sh
echo date=$DATE
echo url=$URL
echo sparkVersion=$SPARK_VERSION
echo hailVersion=$HAIL_VERSION
}

mkdir -p src/main/resources/

echo_build_properties $1 $2 > "src/main/resources/build-info.properties"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this echo function business is a bit unusual. I'd just use the

cat <<DONE
blah blah blah
DONE

idiom. But this is really an unrelated change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's what I would have done too

@tpoterba
Copy link
Contributor Author

uh, this worked locally -- what shell is the CI server running? Can I change "sh" to "bash"?

@tpoterba
Check Python version vs JAR version
8e13266
@tpoterba
Check Python version vs JAR version
318275a 
Also check for a HAIL_IGNORE_PYTHON_VERSION environment variable
so that our development experience doesn't need to be impacted.
@tpoterba
Different approach
4cbe3b1
@tpoterba
generate python version in bash script
f5b87b4
@tpoterba
change to bash
879afa0
@danking danking merged commit 56f890c into hail-is:master Sep 19, 2018
@tpoterba tpoterba deleted the python-version branch September 19, 2018 21:45
danking pushed a commit that referenced this pull request Sep 24, 2018
@cseed @danking
Merge scorecard (#4401)
c0f8585 
* initial commit

* changes to run on cluster

* support multiple repos

put token in kubenetes secrets

* fixed deployment

* fix ci fq repo name

* updated Makefile

use delete/create for redeploy until we stop using latest image

* added custom user pages

* fixed indentation

* fix treating pulls as issues

* poll Github

* fix import

* fix changes requested reporting

* Small improvements

 - declare language="en" so I don't get translate notifications ?????
 - use defaultdict for great profit
 - use daemon thread so ctrl-C actually kills the server

* fix

* Small improvement 2

* add logging, restart on poll thread

* fix users head/header

* autoformat htmls (#12)

* BANISH THE SERIFS WHENCE THEY CAME (#13)

* add author column to "needs review" table (#14)

* Add a list of failing builds to the user page (#16)

* Slightly better formatting. (#17)

Removed headers on tables in the user page

* import sys so the retry stuff works (#18)

* assertion is going off, log instead (#19)

* reverse reviews (come in chrological order) (#20)

We want the most recent.  From https://developer.github.com/v3/pulls/reviews/:

 > The list of reviews returns in chronological order.

scorecard was categorizing #4328 incorrectly.

* move to project dir to merge into monorepo

* updated .gitignore

* backed off spacing

* make targets phony
@tpoterba tpoterba restored the python-version branch November 7, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cseed cseed cseed approved these changes

Assignees

@cseed cseed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tpoterba @cseed @danking