convexengineering / gplibrary

Useful subsystem models
MIT License
10 stars 11 forks source link

Prop #157

Closed courtin closed 6 years ago

courtin commented 6 years ago

Adding simple propeller model with fixed weight and radius.

bqpd commented 6 years ago

it looks like this is legitimately not passing tests, I'll take a look

bqpd commented 6 years ago

@mjburton11 @courtin see code review above (deleting comments related to #24 to make this PR easier to read)

mjburton11 commented 6 years ago

test models please

mjburton11 commented 6 years ago

test this please

mjburton11 commented 6 years ago

test models please

mjburton11 commented 6 years ago

@bqpd are unit tests not working right no in gplibrary?

galbramc commented 6 years ago

test models only works on the gpkit repo pull requests. You want "test this" on everything else. At least the tests appear to be working now.

mjburton11 commented 6 years ago

@courtin do you want to include your motor model in this PR or would you rather do it in a separate PR?

galbramc commented 6 years ago

? what is your question about gplibrary? I didn't understand...

galbramc commented 6 years ago

oh wait. This repo is setup to use "test models" @bqpd is that intentional?

mjburton11 commented 6 years ago

This should run two sets of tests: 1) internal gplibrary unit tests defined in the TESTS file and 2) the research model tests. It only seems to be running 2

galbramc commented 6 years ago

hmmm... I wonder if @bqpd and I miss communicated and accidentally deleted the TESTS one from jenkins...

mjburton11 commented 6 years ago

Ahh that would make sense

galbramc commented 6 years ago

Do you you the command you expect to be run by the "TESTS". This is what is run by "research model tests"

python -c "from gpkit.tests.test_repo import test_repos; test_repos(xmloutput=True, ingpkitmodels=True)"
bqpd commented 6 years ago

Yeah, research model became the TESTS, but gplibrary_PR_models should still be doing test_repos!

On Wed, Mar 28, 2018, 09:28 Marshall Galbraith notifications@github.com wrote:

Do you you the command you expect to be run by the "TESTS". This is what is run by "research model tests"

python -c "from gpkit.tests.test_repo import test_repos; test_repos(xmloutput=True, ingpkitmodels=True)"

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/convexengineering/gplibrary/pull/157#issuecomment-376886489, or mute the thread https://github.com/notifications/unsubscribe-auth/ABagGCUfMnWSjayceE4h9IvyPAeiPTD8ks5ti4_2gaJpZM4S6JcH .

courtin commented 6 years ago

@mjburton11 I think it makes sense as a separate PR

bqpd commented 6 years ago

@galbramc naw this is working as intended; research model tests runs test_repos, while TESTS runs test_repo

galbramc commented 6 years ago

@bqpd, is gpkit_ResearchModel_gplibrary_PR supposed to be the same as the other gpkitResearchModel*_PR tests? I.e should it be running the same script?

galbramc commented 6 years ago

all gpkitReseachModel* tests are running

python -c "from gpkit.tests.test_repo import test_repo; test_repo(xmloutput=True)"

but gpkit_ResearchModel_gplibrary_PR is running

echo "from gpkit.tests.test_repo import test_repo; test_repo('./.', xmloutput=True)" > jenkins_test.py
python $COVERAGE run --append --source=. jenkins_test.py

Is this what you expect? Or should they be the same?

mjburton11 commented 6 years ago

I don't understand the failures here...

galbramc commented 6 years ago

The PR scripts needed some updating. We fixed all the Push scripts, but the PR ones are separate. Try again.

mjburton11 commented 6 years ago

test models please

galbramc commented 6 years ago

test this please

mjburton11 commented 6 years ago

So research model tests timed out after 60 min :( why is it taking so long...

mjburton11 commented 6 years ago

Get getting weird errors with TESTS

galbramc commented 6 years ago

test this please

galbramc commented 6 years ago

apparently I cannot trigger tests here.

I don't know why they take so long. What is the test actually doing?

galbramc commented 6 years ago

I think I fixed the errors, but @bqpd I would still like to know if gpkit_ResearchModel_gplibrary_PR is really supposed to be different from the other ReseachModel tests.

bqpd commented 6 years ago

It looks like everything takes about five times longer; compare win10 timing with win7 timing. So it's not just an interruption. Might it be bad processor scheduling, i.e. the jobs are being overwhelmed by ESP jobs?

galbramc commented 6 years ago

Yeah there were a lot of ESP things running on the node. Strange...

What about the script consistency?

bqpd commented 6 years ago

67 took 31 minutes, 64 took 23, 62 took 25...the runtimes on win7 seem bimodal, with peaks at ~25 minutes and at an hour or more

galbramc commented 6 years ago

I'm guessing it was the load on the machine. What about the scripts being different?

bqpd commented 6 years ago

It's 19/40/40/a very lucky 60/an unlucky 60/25/36/unlucky 60 on gpkit models testing, so it's nearly the same

galbramc commented 6 years ago

What about the script?

bqpd commented 6 years ago

The script is very similar on that test

galbramc commented 6 years ago

But can it be the same? Are the difference needed? If the differences do not matter we can add it to the templating for the others so we have one less script to maintain.

bqpd commented 6 years ago

One of them needs to install local gpkit and master gplibrary, the other needs to the the other, and they call test_repos with one flag different so that gpklibrary is not reinstalled...I think they need to be separate unfortunately.

galbramc commented 6 years ago

No not those. Are these differences signifincant:

python -c "from gpkit.tests.test_repo import test_repo; test_repo(xmloutput=True)"

vs

python $PIP install -v --no-cache-dir --no-deps -e $WORKSPACE
echo "from gpkit.tests.test_repo import test_repo; test_repo('./.', xmloutput=True)" > jenkins_test.py
python $COVERAGE run --append --source=. jenkins_test.py
bqpd commented 6 years ago

Oh! Sorry! I missed that you meant gpkit_ResearchModel_gplibrary_PR, though I see that now when scanning back up. You're totally right, it can inherit from gpkit_ResearchModel. Although we might need to add that python $PIP install -v --no-cache-dir --no-deps -e $WORKSPACE line to ResearchModel if it's not already in there.

galbramc commented 6 years ago

Should all research models be installed? I can't tell if the prop failure is due to this PR or due to the lack of the install.

bqpd commented 6 years ago

The prop failure is definitely due to this PR, since this PR is what introduces that test.

galbramc commented 6 years ago

So if you decide you need to install just the gplibrary, you can add an if test to the gpkit_RearchModel template just for gplibrary test. That is preferable rather than having one more script with a tiny perturbation.

bqpd commented 6 years ago

@galbramc I'm afraid I'm not following what you're saying! Are you proposing a further modification?

mjburton11 commented 6 years ago

I don't understand why it think prop is not a module...

Do we just need to increase the run time of the research tests? That seems to be the only reason it is failing.

bqpd commented 6 years ago

increased the timeout, though it's too bad it takes so long!

mjburton11 commented 6 years ago

agreed. still no understanding of why TESTS is failing??

mjburton11 commented 6 years ago

test models please

mjburton11 commented 6 years ago

Woohoo! Passed research tests! 1 to @bqpd do you know what is going on?

galbramc commented 6 years ago

I added more cores to the Windows 7 VM. Hopefully it runs faster now.

mjburton11 commented 6 years ago

@galbramc do you know why this is still failing TESTS?