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: Split bear execution by runtime #1386

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jayvdb
Copy link
Member

@jayvdb jayvdb commented Feb 1, 2017

Closes #1773

@gitmate-bot
Copy link
Collaborator

Thanks for your contribution!

Reviewing pull requests take really a lot of time and we're all volunteers. Please make sure you go through the following check list and complete them all before pinging someone for a review.

As you learn things over your Pull Request please help others on the chat and on PRs to get their stuff right as well!

@jayvdb
Copy link
Member Author

jayvdb commented Feb 1, 2017

This is mostly a WIP,, as it needs to be split into multiple commits and some of the voodoo bash stuff needs to be refined, and some additional Requirement need to be added for the current algorithm to correctly split Python bears from non-Python bears.

However, the concept behind it, of not testing non-Python bears on each version of Python, needs some arch review.
The main performance improvement is that any job which doesnt require sudo can run start (and finish) much faster, which gives better feedback for many times of -bear work. Due to the PPAs, some jobs need to use sudo, and these wont run any faster, so the actual elapsed completion time wont be reduced, at least not much.

There are many further improvements that can be made, such as splitting node and ruby and python bears into separate jobs (these are already manageable by Requirement classes), and R and Haskell and Java should follow soon with Requirement classes being added. That will allow testing using multiple versions of each runtime concurrently.

@sils
Copy link
Member

sils commented Feb 1, 2017

@jayvdb gorgeous idea! Put th concept explanation into the commit message please or maybe right into the code as a comment.

# Delete empty bear directories

for dir in bears/*/; do
if [[ -z "$(ls $dir/ 2>/dev/null | egrep -v '(__|.xml)')" ]]; then
Copy link
Member Author

Choose a reason for hiding this comment

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

So this looks for directories without any Bear's in it..

The __ is for __init__ and __pycache__
.xml is because of bears/scala/scalastyle_config.xml

I think a much better approach would be to look for one file matching *Bear.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the problem with *Bear is we have directories with only subdirectories, like bears/vcs. Solvable, and probably still clearer to take this approach.

.travis.yml Outdated
if [[ "$BEARS" != "all" ]]; then
rm $(comm -23 <(ls bears/*/[A-Za-z]*.py | grep -v general | sort) <(grep -l PipRequirement bears/*/*.py | sort))
rm bears/general/CPDBear.py
rm bears/generate_package.py tests/generate_packageTest.py
Copy link
Member Author

Choose a reason for hiding this comment

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

the special case for bears/generate_package.py -> tests/generate_packageTest.py could be added to tests.prune.sh, instead of here in .travis.yml

.travis.yml Outdated
# Only keep non-Python bears on Python 3.4
- >
if [[ "$BEARS" != "all" ]]; then
rm $(comm -23 <(ls bears/*/[A-Za-z]*.py | grep -v general | sort) <(grep -l PipRequirement bears/*/*.py | sort))
Copy link
Member Author

Choose a reason for hiding this comment

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

grep -v general -- all bears/general/ are Python language bears without any dependencies, except CPDBear (see next line).

This needs to be tidied up somehow.

@@ -19,8 +19,7 @@ class RuboCopBear:
"""

LANGUAGES = {'Ruby'}
REQUIREMENTS = {GemRequirement('rubocop'),
PipRequirement('pyyaml', '3.12')}
Copy link
Member Author

Choose a reason for hiding this comment

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

This was done because the shell foo in .travis.yml needs to find non-Python bears. It is using the presence of PipRequirement, and there are two non-Python bears which use PipRequirement.

.travis.yml logic needs to be rewritten, and then this isnt necessary.

@@ -81,6 +82,15 @@ def get_inherited_requirements():
in_inherited = False
with open(os.path.join(PROJECT_DIR, 'requirements.txt'), 'r') as file:
for line in file.read().splitlines():
line = line.strip()
if not line.startswith('#'):
m = re.match('^[A-Za-z._-]+', line)
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding more requirements parsing ;-( Should be using an existing tool for this.
This entire modification to this script wont be necessary when the pyyaml related changes are backed-out.

@gitmate-bot
Copy link
Collaborator

Comment on e320695.

This commit seems to be marked as work in progress and should not be used in production. Treat carefully.

GitCommitBear, severity NORMAL, section commit.

@jayvdb jayvdb force-pushed the travis-matrix-python-speedup branch from e320695 to 1b8458a Compare February 3, 2017 17:21
@gitmate-bot
Copy link
Collaborator

Comment on 1b8458a.

This commit seems to be marked as work in progress and should not be used in production. Treat carefully.

GitCommitBear, severity NORMAL, section commit.

1 similar comment
@gitmate-bot
Copy link
Collaborator

Comment on 1b8458a.

This commit seems to be marked as work in progress and should not be used in production. Treat carefully.

GitCommitBear, severity NORMAL, section commit.

@jayvdb jayvdb changed the title Only test non-Python bears on Python 3.6 Split bear execution per runtime Feb 4, 2017
@jayvdb jayvdb changed the title Split bear execution per runtime Split bear execution by runtime Feb 4, 2017
@jayvdb
Copy link
Member Author

jayvdb commented Feb 4, 2017

Note bca64a0 was an intentional breakage to show codecov reports less than 100% coverage when one bear doesnt work. See it on
https://codecov.io/gh/jayvdb/coala-bears/branch/travis-matrix-python-speedup/commits

@jayvdb jayvdb force-pushed the travis-matrix-python-speedup branch 2 times, most recently from 585ccdb to 921ebc6 Compare February 4, 2017 01:10
@jayvdb
Copy link
Member Author

jayvdb commented Feb 6, 2017

The circle breakage has been addressed in #1406 , which only solves part of this as it is easier to test that part by itself.

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

@gitmate-bot
Copy link
Collaborator

Hey! This pull request hasn't been updated for a while :/ It would be nice if we could get this going again!

go_requirement_bears=$(grep -m 1 -l GoRequirement $requirement_bears)
npm_requirement_bears=$(grep -m 1 -l NpmRequirement $requirement_bears)
rscript_requirement_bears=$(grep -m 1 -l RscriptRequirement $requirement_bears)

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add luarocks_requirement_bears and julia_requirement_bears

Ref: #1650


dart_bears=$(ls bears/dart/*Bear.py)
julia_bears=$(ls bears/julia/*Bear.py)
lua_bears=$(ls bears/lua/*Bear.py)
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove lua_bears and julia_bears


non_python_bears=$(comm -23 <(ls $bears) <(ls $python_bears))

cabal_requirement_bears="$cabal_requirement_bears bears/haskell/HaskellLintBear.py bears/shell/ShellCheckBear.py"
Copy link
Contributor

Choose a reason for hiding this comment

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

ShellCheckBear has a Cabal Requirement now

Ref: #1496

Copy link
Contributor

@saksham189 saksham189 Apr 12, 2018

Choose a reason for hiding this comment

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

Also this needs to be solved: #1358 so, that we can remove HaskellLintBear

if [[ -z "$bears""$subdirs" ]]; then
echo Removing $dir
rm -rf $dir
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it important to delete the empty directories? What if we left them as it is?

Copy link
Member Author

Choose a reason for hiding this comment

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

I forget.

probably another bear will complain about a missing __init__.py, or we have missing coverage of those __init__.py

@@ -0,0 +1,25 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

DESCRIPTION Outdated
@@ -0,0 +1,3 @@
Package: coala
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,6 @@
use ExtUtils::MakeMaker;
Copy link
Member Author

Choose a reason for hiding this comment

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

if [[ -z "$bears""$subdirs" ]]; then
echo Removing $dir
rm -rf $dir
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

I forget.

probably another bear will complain about a missing __init__.py, or we have missing coverage of those __init__.py


yield_result_bears=$(grep -m 1 -l 'yield Result' $bears)

non_yield_result_bears=$(comm -23 <(ls $bears) <(ls $yield_result_bears))
Copy link
Collaborator

Choose a reason for hiding this comment

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

non_yield_result_bears appears unused. Verify it or export it. [SC2034]

Origin: ShellCheckBear, Section: bash.

fi

pip_only_requirement_bears=$(comm -23 <(ls $pip_requirement_bears) <(ls $non_pip_runtime_requirement_bears))
pip_plus_requirement_bears=$(comm -23 <(ls $pip_requirement_bears) <(ls $pip_only_requirement_bears))
Copy link
Collaborator

Choose a reason for hiding this comment

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

pip_plus_requirement_bears appears unused. Verify it or export it. [SC2034]

Origin: ShellCheckBear, Section: bash.

clang_bears=''
other_bears=''
for bear in $no_requirement_bears; do
if [[ ${bear/Clang/} != $bear ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quote the rhs of != in [[ ]] to prevent glob matching. [SC2053]

Origin: ShellCheckBear, Section: bash.

@jayvdb jayvdb force-pushed the travis-matrix-python-speedup branch from 9fe134a to 041f05b Compare November 12, 2018 09:10
clang_bears=''
other_bears=''
for bear in $no_requirement_bears; do
if [[ ${bear/Clang/} != ${bear} ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quote the rhs of != in [[ ]] to prevent glob matching. [SC2053]

Origin: ShellCheckBear, Section: bash.

@jayvdb jayvdb force-pushed the travis-matrix-python-speedup branch 3 times, most recently from d56fc7f to d28fca8 Compare November 12, 2018 09:39
- language: ruby
rvm: 2.0

- language: haskell
Copy link
Member Author

Choose a reason for hiding this comment

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

this job isnt working yet in the new PR #2910

the TRAVIS_GHC_ROOT=/opt/ghc env setting below might be relevant.


config = configparser.ConfigParser()
config.read('setup.cfg')
if 'setup:custom' in config.sections():
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a separate issue, and hasnt been fixed yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants