Skip to content

Conversation

@Borda
Copy link
Collaborator

@Borda Borda commented Dec 7, 2020

What does this PR do?

enable DDP test for example

Resolves #4694

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda added example ci Continuous Integration priority: 2 Low priority task labels Dec 7, 2020
@Borda Borda added this to the 1.1.x milestone Dec 7, 2020
@Borda
Copy link
Collaborator Author

Borda commented Dec 7, 2020

seems that the DDP is hanging...

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Drone are breaking. When resolved, LGTM !

@Borda Borda marked this pull request as ready for review December 8, 2020 08:48
@awaelchli
Copy link
Contributor

@Borda by how much will this increase drone test runtime?

@justusschock
Copy link
Member

I think, we should only run this nightly with a cron job or so. Because when we run this more often, we will significantly slow down the development process, as we already experience some bottlenecks with iron and then it would pile up even more.

@Borda
Copy link
Collaborator Author

Borda commented Dec 9, 2020

I think, we should only run this nightly with a cron job or so. Because when we run this more often, we will significantly slow down the development process, as we already experience some bottlenecks with iron and then it would pile up even more.

well then you can say the same for each test we run, we can run a test just nightly :]
if we want some simplification we shall skip all code testing for pure doc changes...

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

@mergify mergify bot requested a review from a team December 12, 2020 14:57
@justusschock
Copy link
Member

I think, we should only run this nightly with a cron job or so. Because when we run this more often, we will significantly slow down the development process, as we already experience some bottlenecks with iron and then it would pile up even more.

well then you can say the same for each test we run, we can run a test just nightly :]
if we want some simplification we shall skip all code testing for pure doc changes...

I don't think so. There's a difference in running unit tests and examples (which are basically some kind of integration/functionality testing).

@awaelchli
Copy link
Contributor

I very much agree with Justus on this one

@Borda
Copy link
Collaborator Author

Borda commented Dec 17, 2020

I don't think so. There's a difference in running unit tests and examples (which are basically some kind of integration/functionality testing).

so you say we shall drop the test and simply call the script in a command-line as another entry after python -m pytest

@Borda Borda requested a review from tchaton December 17, 2020 16:16
@Borda Borda marked this pull request as draft December 17, 2020 16:16
@Borda
Copy link
Collaborator Author

Borda commented Dec 19, 2020

@justusschock @awaelchli this is what you had in mind b26adda?

@Borda Borda force-pushed the tests/examples-ddp branch from b26adda to 93655a3 Compare December 19, 2020 12:45
@awaelchli
Copy link
Contributor

awaelchli commented Dec 19, 2020

it looks like this would still run on every commit/PR?

@codecov
Copy link

codecov bot commented Dec 19, 2020

Codecov Report

Merging #4995 (eb54f18) into master (b295029) will decrease coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4995   +/-   ##
======================================
- Coverage      90%     90%   -0%     
======================================
  Files         170     170           
  Lines       11807   11807           
======================================
- Hits        10676   10646   -30     
- Misses       1131    1161   +30     

@Borda
Copy link
Collaborator Author

Borda commented Dec 21, 2020

it looks like this would still run on every commit/PR?

yes, or what is your suggestion, just at midnight, but then we do not know what PR breaks it... (well, stop it which would break it...)

@Borda Borda self-assigned this Dec 29, 2020
@Borda
Copy link
Collaborator Author

Borda commented Dec 29, 2020

@justusschock @awaelchli so whall we keep this DDP testing or just drop it to run it only on master?

@Borda Borda changed the base branch from master to release/1.2-dev February 8, 2021 12:11
@Borda Borda modified the milestones: 1.1.x, 1.2 Feb 8, 2021
@edenlightning edenlightning modified the milestones: 1.2, 1.2.x Feb 8, 2021
@Borda Borda force-pushed the tests/examples-ddp branch from d5423c6 to 6ec541a Compare February 9, 2021 09:24
Base automatically changed from release/1.2-dev to master February 11, 2021 14:31
@Borda Borda force-pushed the tests/examples-ddp branch from 6ec541a to e4110a3 Compare February 12, 2021 08:21
@Borda Borda marked this pull request as ready for review February 12, 2021 16:52
@Borda Borda requested a review from carmocca as a code owner February 12, 2021 16:52
@Borda Borda enabled auto-merge (squash) February 12, 2021 17:08
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

@Borda Borda force-pushed the tests/examples-ddp branch from f3c1635 to 978bdf2 Compare February 12, 2021 20:58
@Borda Borda added the ready PRs ready to be merged label Feb 13, 2021
@Borda Borda merged commit ba806c8 into master Feb 15, 2021
@Borda Borda deleted the tests/examples-ddp branch February 15, 2021 15:36
@Borda Borda modified the milestones: 1.2.x, 1.2 Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Continuous Integration example ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix failing examples

7 participants