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: install error due to peer conflict #611

Closed
wants to merge 1 commit into from

Conversation

ghassanmas
Copy link
Member

@ghassanmas ghassanmas commented Jun 27, 2022

This fixes install error on node 16 and npm => 8.6.*
The install error is due to confliect of peer dependecy.
The project is using tensorflow-models/blazeface 0.0.7
which require tensorflow/tfjs-converter and tensorflow/tfjs-core
to be ^3.11.0 ref

Note: this might not have caused erros before, because probably npm older version didn't catch but it was suppose to.
Also this needs probably need to be backported to nutmeg.

The error:

#35 9.604 npm ERR! code ERESOLVE
#35 9.609 npm ERR! ERESOLVE could not resolve
#35 9.611 npm ERR!
#35 9.612 npm ERR! While resolving: @tensorflow-models/blazeface@0.0.7
#35 9.613 npm ERR! Found: @tensorflow/tfjs-converter@1.7.4
#35 9.614 npm ERR! node_modules/@tensorflow/tfjs-converter
#35 9.614 npm ERR!   @tensorflow/tfjs-converter@"1.7.4" from the root project
#35 9.615 npm ERR!
#35 9.616 npm ERR! Could not resolve dependency:
#35 9.617 npm ERR! peer @tensorflow/tfjs-converter@"^3.1.0" from @tensorflow-models/blazeface@0.0.7
#35 9.617 npm ERR! node_modules/@tensorflow-models/blazeface
#35 9.618 npm ERR!   @tensorflow-models/blazeface@"0.0.7" from the root project
#35 9.619 npm ERR!
#35 9.620 npm ERR! Conflicting peer dependency: @tensorflow/tfjs-converter@3.18.0
#35 9.622 npm ERR! node_modules/@tensorflow/tfjs-converter
#35 9.623 npm ERR!   peer @tensorflow/tfjs-converter@"^3.1.0" from @tensorflow-models/blazeface@0.0.7
#35 9.624 npm ERR!   node_modules/@tensorflow-models/blazeface
#35 9.625 npm ERR!     @tensorflow-models/blazeface@"0.0.7" from the root project
#35 9.626 npm ERR!
#35 9.627 npm ERR! Fix the upstream dependency conflict, or retry
#35 9.627 npm ERR! this command with --force, or --legacy-peer-deps
#35 9.628 npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
#35 9.629 npm ERR!
#35 9.630 npm ERR! See /root/.npm/eresolve-report.txt for a full report.
#35 9.633
#35 9.633 npm ERR! A complete log of this run can be found in:
#35 9.634 npm ERR!     /root/.npm/_logs/2022-06-27T10_39_41_571Z-debug-0.log

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 27, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @ghassanmas! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@ghassanmas
Copy link
Member Author

ghassanmas commented Jun 27, 2022

Why this doesn't occur in edx: Because edx is using an older version of npm which doesn't check for validation, hence

  • npm version for edxapp is 8.5.0.

  • npm veriosn for MFEs 8.1.2

Given since npm 8.6.0 or something npm started to validate pacakge-lock.json hence: npm/cli#4664

@adamstankiewicz
Copy link
Member

@ghassanmas The upgrade of these tensorflow packages is going from v1 to v3. Were there any breaking changes that needed to be addressed in this repo, or are the breaking changes in v2/v3 not relevant to this repo?

Also, FWIW, it looks like this repo has a is-es5 check as part of its CI. This can be changed to actually validate against is-es6 instead since we've dropped the requirement to support older browsers like IE 11 that rely on ES5-compatible code.

@ghassanmas
Copy link
Member Author

@ghassanmas The upgrade of these tensorflow packages is going from v1 to v3. Were there any breaking changes that needed to be addressed in this repo, or are the breaking changes in v2/v3 not relevant to this repo?

It shouldn't because to my understanding This repo doesn't use tensorflow core nor tensorflow converter, they are added just as peer dependecy only I assume?. When I searched in repo code, I can only find usage only for '@tensorflow-models/blazeface' which is not updated in this PR. For reference here the official notes about the releases:

Also, FWIW, it looks like this repo has a is-es5 check as part of its CI. This can be changed to actually validate against is-es6 instead since we've dropped the requirement to support older browsers like IE 11 that rely on ES5-compatible code.

I guess you mean here it needs simlair PR to this. Should I make simliar changes and then rebase this PR after is-es5 is removed?

@dcoa
Copy link
Contributor

dcoa commented Jul 12, 2022

Hello, I think is-es5 is already removed 1967199, and now you can rebase this branch to pass the test

  This fixes install error on node 16 and npm => 12
  The install error is due to confliect of peer dependecy.
  The project is using tensorflow-models/blazeface 0.0.7
  which require tensorflow/tfjs-converter and tensorflow/tfjs-core
  to be ^3.11.0 ref:
  https://github.com/tensorflow/tfjs-models/blob/dcec078d66eb36e36cae516e1581d69d50d4fca3/blazeface/package.json#L15-L18
  Note: this might not have caused erros before, because probably
  npm older version didn't catch but it was suppose to.
@ghassanmas
Copy link
Member Author

@dcoa thanks for the update!; its done now

@natabene
Copy link

@ghassanmas Thank you for your contribution.

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #611 (f850946) into master (1967199) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #611   +/-   ##
=======================================
  Coverage   39.53%   39.53%           
=======================================
  Files         106      106           
  Lines        2188     2188           
  Branches      582      582           
=======================================
  Hits          865      865           
  Misses       1238     1238           
  Partials       85       85           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1967199...f850946. Read the comment docs.

@dcoa
Copy link
Contributor

dcoa commented Jul 28, 2022

Hi @ghassanmas I think this PR could be closed due to someone updating the @tensorflow dependency and is in master now 0527c73

@ghassanmas ghassanmas closed this Jul 28, 2022
@openedx-webhooks
Copy link

@ghassanmas Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@ghassanmas ghassanmas deleted the tensorflow-peer branch July 28, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants