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

add PR/MR comments #776

Merged
merged 11 commits into from
Oct 27, 2021
Merged

add PR/MR comments #776

merged 11 commits into from
Oct 27, 2021

Conversation

DavidGOrtega
Copy link
Contributor

@DavidGOrtega DavidGOrtega commented Oct 24, 2021 •
edited
Loading

  • closes support PR comments #735
  • Avoid request of comments even if no update is needed
  • Dryer building the business logic in CML and not one for every single driver
  • Standardises driver return (comment url)

@DavidGOrtega DavidGOrtega temporarily deployed to internal October 24, 2021 17:32 Inactive
@DavidGOrtega DavidGOrtega self-assigned this Oct 24, 2021
@DavidGOrtega DavidGOrtega marked this pull request as draft October 24, 2021 17:32
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 24, 2021 19:53 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 24, 2021 23:15 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 24, 2021 23:50 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 00:00 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 00:16 Inactive
@DavidGOrtega DavidGOrtega marked this pull request as ready for review October 25, 2021 00:30
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 00:30 Inactive
@DavidGOrtega
Copy link
Contributor Author

DavidGOrtega commented Oct 25, 2021 •
edited
Loading

@0x2b3bfa0 is ready for review, however seems that TB tests are failing, not because of this PR I might say.
You could start to review it in the meantime we discover whats wrong with TB

@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 08:57 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 09:31 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 11:07 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 11:34 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 11:37 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 11:37 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 11:50 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 12:00 Inactive
@DavidGOrtega
Copy link
Contributor Author

TB tests are failing due to python 3.10.
We are fixing that in #777

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal October 25, 2021 16:17 Inactive
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 16:43 Inactive
@0x2b3bfa0
Copy link
Member

@DavidGOrtega, are you inadvertently using @iterative-olivaw's identity for your commits? 🤔

@casperdcl casperdcl self-requested a review October 25, 2021 18:39
DavidGOrtega and others added 4 commits October 25, 2021 21:54
@DavidGOrtega
fix tests
5650d5d
@snyk-bot @0x2b3bfa0 @DavidGOrtega
[Snyk] Upgrade pseudoexec from 0.1.4 to 0.2.0 (#755)
9b36362 
* fix: upgrade pseudoexec from 0.1.4 to 0.2.0

Snyk has created this PR to upgrade pseudoexec from 0.1.4 to 0.2.0.

See this package in npm:
https://www.npmjs.com/package/pseudoexec

See this project in Snyk:
https://app.snyk.io/org/casperdcl/project/58d0fcac-fe96-4a3b-b587-5fa62067cfa1

Note from @0x2b3bfa0: incidentally, this package is my brainchild and the only major difference between earlier versions and 0.2 is tests.
https://github.com/0x2b3bfa0/node-pseudoexec

Co-authored-by: Helio Machado <0x2b3bfa0+git@googlemail.com>
@DavidGOrtega
long report in prs
ca6c60f
@DavidGOrtega @iterative-olivaw @casperdcl
fix tensorboard py3.10 (#777)
97a1442 
* Fixes TB python 3.10 failure

* py3.9

Co-authored-by: Olivaw[bot] <olivaw@iterative.ai>
Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 25, 2021 19:57 Inactive
@DavidGOrtega
Copy link
Contributor Author

I know why, inside a linux machine where Im developing a new cml pr feature cml-pr has changed the user.
cml-pr should set back the original user for non CI use cases (im actually developing one with the chrome extension)

@casperdcl

This comment has been minimized.

Sign in to view
casperdcl
casperdcl suggested changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

🎉 minor comments

casperdcl added a commit to iterative/cml.dev that referenced this pull request Oct 26, 2021
@casperdcl
send-comment --pr
acf4f94 
related to iterative/cml#776
@casperdcl casperdcl mentioned this pull request Oct 26, 2021
Merged
1 task
0x2b3bfa0
0x2b3bfa0 previously approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal October 27, 2021 00:35 Inactive
@DavidGOrtega @casperdcl
Cli docs and minors
03bba2b 
Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>
@DavidGOrtega DavidGOrtega temporarily deployed to internal October 27, 2021 06:39 Inactive
casperdcl
casperdcl approved these changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

lgtm 🙏

@casperdcl casperdcl self-requested a review October 27, 2021 09:40
casperdcl
casperdcl suggested changes Oct 27, 2021
View reviewed changes
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

actually GL tests failing now :/

@DavidGOrtega
Copy link
Contributor Author

actually GL tests failing now :/

sometimes the onprem GL is not ready and tests must be repeated. We have been dealing with that

@casperdcl casperdcl self-requested a review October 27, 2021 09:52
@casperdcl casperdcl changed the title PR comment add PR/MR comments Oct 27, 2021
@casperdcl casperdcl merged commit 0958298 into master Oct 27, 2021
@casperdcl casperdcl deleted the pr-comment branch October 27, 2021 09:53
This was referenced Oct 28, 2021
Merged
Closed
This was referenced Mar 29, 2022
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperdcl casperdcl casperdcl approved these changes

@0x2b3bfa0 0x2b3bfa0 0x2b3bfa0 left review comments

Assignees

@DavidGOrtega DavidGOrtega

Labels
ci-bitbucket ci-github ci-gitlab cml-comment Subcommand enhancement New feature or request p1-important High priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support PR comments
4 participants
@DavidGOrtega @0x2b3bfa0 @casperdcl @snyk-bot