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

fix build in Travis #1137

Closed
wants to merge 7 commits into from
Closed

fix build in Travis #1137

wants to merge 7 commits into from

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Oct 12, 2019

drop testing models because of the timeout failer
start adding CircleCI which should be able to run full test

@codecov-io
Copy link

codecov-io commented Oct 12, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a5d6d01). Click here to learn what that means.
The diff coverage is 67%.

@@           Coverage Diff            @@
##             master   #1137   +/-   ##
========================================
  Coverage          ?     66%           
========================================
  Files             ?      41           
  Lines             ?    2335           
  Branches          ?       0           
========================================
  Hits              ?    1532           
  Misses            ?     803           
  Partials          ?       0

@Borda Borda changed the title fix build in Travis fix build in Travis [WIP] Oct 12, 2019
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@hgaiser
Copy link
Contributor

hgaiser commented Oct 14, 2019

@Borda this might be of interest to you: #1123 (comment)

@Borda
Copy link
Contributor Author

Borda commented Oct 14, 2019

thx, I will try to use it in testing if it helps... but it means that we restrict to TF usage only, right?

import tensorflow as tf
tf.compat.v1.disable_v2_behavior()

@Borda
Copy link
Contributor Author

Borda commented Oct 15, 2019

CircleCI

@Borda
Copy link
Contributor Author

Borda commented Oct 15, 2019

Still failing on CircleCI...

@hgaiser
Copy link
Contributor

hgaiser commented Oct 17, 2019

A general question, why should we use CircleCI instead of Travis?

@de-vri-es do you have any experience with CircleCI vs Travis?

@Borda
Copy link
Contributor Author

Borda commented Oct 17, 2019

I think that both Travis and CircleCI can be used in parallel... But I agree that it is not necessary so I will remove CircleCI from this PR
From my experience CircleCI has better machines and the tests runs faster, about 20%

@de-vri-es
Copy link
Contributor

@de-vri-es do you have any experience with CircleCI vs Travis?

I think that both Travis and CircleCI can be used in parallel... But I agree that it is not necessary so I will remove CircleCI from this PR
From my experience CircleCI has better machines and the tests runs faster, about 20%

I have very limited experience with CircleCI. They have some cool features (you can get a shell in the VM/container of a failed run, for example), but I think we should stick with one CI system in total.

I'd be fine with either CircleCI or Travis. If it really runs faster that could be a good reason to switch. But whichever we go with, they must be able to complete the test suite =]

@Borda Borda changed the title fix build in Travis [WIP] fix build in Travis Oct 19, 2019
@Borda
Copy link
Contributor Author

Borda commented Oct 21, 2019

@de-vri-es I have stick this PR just with Travis and I may create a new PR with CircleCI and clearly state if and what is the performance boost compare to Travis...

@de-vri-es
Copy link
Contributor

@de-vri-es I have stick this PR just with Travis and I may create a new PR with CircleCI and clearly state if and what is the performance boost compare to Travis...

Makes sense 👍

@Borda
Copy link
Contributor Author

Borda commented Oct 21, 2019

@de-vri-es is there anything else I should do for this PR? :)

@de-vri-es
Copy link
Contributor

I think we want to merge most of this, but I'm not so sure about disabling the tests, even if it is temporary. We're close to a fix in #1146, and I'm a bit afraid that if we disable the tests now, we won't enable them again soon.

@hgaiser, @vcarpani: What do you think?

@hgaiser
Copy link
Contributor

hgaiser commented Oct 23, 2019

I agree with @de-vri-es, we're hopefully close to having working unit tests in keras 2.3 so we should aim for that.

@vcarpani
Copy link
Contributor

Let's wait for working unit tests in Keras 2.3, it should not take too much time

@Borda
Copy link
Contributor Author

Borda commented Oct 24, 2019

I would consider adding to CI testing for Tensorflow 1.x and 2.x

@Borda
Copy link
Contributor Author

Borda commented Oct 29, 2019

@vcarpani @de-vri-es @hgaiser I have implemented the behaviour change and run multi TF testing...
in my opinion, this implementation of v1 behaviour is much smoother :]
BUT it seems that it needs also enable eager (as mentioned #1123 (comment)) which makes densenet freeze - https://travis-ci.org/Borda/keras-retinanet/jobs/604432952#L924

@Borda
Copy link
Contributor Author

Borda commented Dec 3, 2019

@de-vri-es @hgaiser @vcarpani any update so far? :]

@hgaiser
Copy link
Contributor

hgaiser commented Dec 5, 2019

Nope, seems keras is dead.

@Borda
Copy link
Contributor Author

Borda commented Dec 5, 2019

@hgaiser does it mean that this repo is also dead?

@hsahin
Copy link

hsahin commented Dec 5, 2019

@hgaiser ?

@hgaiser
Copy link
Contributor

hgaiser commented Dec 5, 2019

It will be deprecated in time, in favor of the implementation using tf.keras. For now it's not dead.

@Borda Borda closed this Mar 18, 2020
@Borda Borda deleted the travis branch September 3, 2020 20:30
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.

6 participants