[Movey] Call Movey API to increase download count #318

ea-open-source · 2022-07-29T02:11:31Z

Motivation

According to Sam, we moved the call to Movey into this pull request.

How to use it

As usual any build action that involves cloning from github will also calls to Movey to increase download count metre.

Note: you could use the flag--skip-movey to skip this call, although it has negligible impact on build performance.
Example: move build --skip-movey

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

cargo test --package move-package --lib increase_movey_download_count

awelc

I have to say I am still confused what this PR does... I though this addition is to avoid increasing GitHub's download count when uploading the repo content to Movey (so that the counter value is not artificially inflated by multiple uploads). Looking at the code, though, it seems like we are actually incrementing the counter (which would otherwise not be incremented as part of the upload?), so I am not even sure if I got the notion of the counter right... Could you please provide some additional explanation?

ea-open-source · 2022-08-17T03:19:01Z

I have to say I am still confused what this PR does... I though this addition is to avoid increasing GitHub's download count when uploading the repo content to Movey (so that the counter value is not artificially inflated by multiple uploads). Looking at the code, though, it seems like we are actually incrementing the counter (which would otherwise not be incremented as part of the upload?), so I am not even sure if I got the notion of the counter right... Could you please provide some additional explanation?

For this PR it does increase the counter but it's the download counter whenever a move package in move.toml is resolved by resolution graph. A new thread will be spun to send a request to Movey whenever download_and_upload_if_remote is called so by default it will skip already downloaded packages.

awelc · 2022-08-17T17:14:39Z

Adding @tnowacki as a reviewer here as this PR involves a change to package resolver and I am not sure if spawning a thread and calling to Movey on (seemingly) each dependency check (for already downloaded dependencies) during resolution graph building is the right strategy here.

ea-open-source · 2022-08-18T08:54:55Z

Adding @tnowacki as a reviewer here as this PR involves a change to package resolver and I am not sure if spawning a thread and calling to Movey on (seemingly) each dependency check (for already downloaded dependencies) during resolution graph building is the right strategy here.

I think you misunderstood the logic, calling to Movey only happens when download_and_update_if_remote is called and this only happens after the dependency tree is resolved. So in essence, Movey only increases download counts when users download a package from remote.

…o `increase_movey_download_count`

…_repo`, rename `skip_movey_flag` to `skip_movey`

- change `init_mock_server` to `mock_movey_count_request_with_response_status_code` - change `wait` to `test_thread_wait`

…te_if_remote`

ea-open-source · 2022-08-31T07:08:47Z

@awelc @tnowacki would you guys help us review this PR?

awelc

Thank you for the additional explanation! It looks like the call to Movey indeed only happens right where download_and_update_if_remote is called, which makes me wonder why you did not include it as part of download_and_update_if_remote function's definition. You even have the right checks there already:

        if let Some(git_info) = &dep.git_info {
            if !git_info.download_to.exists() {

This would make the logic here more explicit right off the bat and I suggest you do it unless there is a good reason not to.

I am still confused by what you are trying to accomplish here. You call into Movey when resolving dependencies during a build process and a developer may be building a package that has nothing to do with Movey. I don't understand why you want to call to Movey in such cases, and even more importantly, even if this was the case, this should be presented as opt-in to the user (they choose to increase the Movey download count) rather than opt-out (they have to choose not to increase the count).

Additionally, the --skip-movey flag is presented in the description of this PR as an addition to the move movey-upload, but it's part of build config and (I think) move movey-upload does not even do any building. Please clarify.

tnowacki · 2022-09-09T17:14:31Z

language/tools/move-package/src/resolution/resolution_graph.rs

+                }
+            }

+            Self::download_and_update_if_remote(*dep_name, dep)?;


I feel like it would make more sense to include this logic in download_and_update_if_remote. Is there some reason not to?

This was our solution originally, however, we decided to move the call to update_download_count outside of the download_and_update_if_remote. We do this in order to write tests and separate Movey logic and download_and_update_if_remote logic as much as possible.

We can move update_download_count logic inside download_and_update_if_remote but doing that would complicate the testing logic.

tnowacki · 2022-09-09T17:15:48Z

language/tools/move-package/src/resolution/resolution_graph.rs

+        if skip_movey {
+            return;
+        }


I feel like it is cleaner to move this to outside of this function. Feels awkward to have a bool flag that is just immediately used to return or not

This can be resolved depending on the solution of #318 (comment)

tnowacki · 2022-09-09T17:16:35Z

language/tools/move-package/src/resolution/resolution_graph.rs

+            if let Some(git_info) = &dep.git_info {
+                if !git_info.download_to.exists() {


Consider also

let already_downloaded = dep.git_info.map(|gi| gi.download_to.exists()).unwrap_or(true); if !already_downloaded { Self::increase_movey_download_count( ... ) }

This can be resolved depending on the solution of #318 (comment)

tnowacki · 2022-09-09T17:17:32Z

language/tools/move-package/src/resolution/resolution_graph.rs

+
+    fn test_thread_wait(wait_time: Option<u64>) {
+        // make sure the spawn thread has enough time to run
+        let thread_sleep_time = time::Duration::from_millis(wait_time.unwrap_or(20));


nit maybe move 20 to a constant

tnowacki · 2022-09-09T17:28:42Z

In the PR description, you have

Example: move movey-upload --skip-movey

I think this should be move build --skip-movey? I'm not sure how this control flow path will ever be hit in movey-upload?

ea-open-source · 2022-09-12T09:28:10Z

@awelc @tnowacki There are multiple reasons why we made this explicitly opt-in:

Most of our existing packages were crawled from Github and they include a lot of unused or for practice packages. We collect these information to generate Movey info page about truly important packages, to set them apart from automatically crawled packages.
In order to do this, we aim to integrate ourselves into the Move ecosystem as the official Move package repository. Therefore we set opt-in as the default behavior.

ea-open-source · 2022-09-12T09:30:33Z

Additionally, the --skip-movey flag is presented in the description of this PR as an addition to the move movey-upload, but it's part of build config and (I think) move movey-upload does not even do any building. Please clarify.

In the PR description, you have

Example: move movey-upload --skip-movey

I think this should be move build --skip-movey? I'm not sure how this control flow path will ever be hit in movey-upload?

Yes, It should be move build --skip-movey not move movey-upload --skip-movey. Our bad.

tnowacki

After discussing it, we have decided it would be best if movey was it's own custom source provider. These were recently added, see #318

This way, movey packages will be resolved with their own syntax, something similar to CustomPackage = { node: "movey", address: "0x1", package: "custom-package" }

lwsinclair · 2022-09-14T02:17:19Z

Movey is a centralized registry for Move packages but not an independent repository ("its own custom source provider"). Among the reasons for not being a repository are (1) developers likely prefer Github and this may hamper adoption, and (2) there would be a lot more engineering involved and this would take time and be redundant ("reinventing the wheel"), (3) this would take focus away from features that add value not found elsewhere.

Perhaps what is being suggested here is that Movey should provide a pointer to existing on-chain locations for packages. And indeed, we do have that on our roadmap. At first these links might be informational (no guarantees) later they can be validated and certified by us.

@tnowacki perhaps we are missing something. What is the compelling advantage?

ea-open-source · 2023-02-17T07:48:01Z

After further discussions about package dependencies resolving, we have released a new tool MoveyCLI. This PR is no longer needed. Closing.

ea-open-source mentioned this pull request Aug 2, 2022

[Movey] Add command to upload metadata to Movey #227

Merged

awelc self-assigned this Aug 3, 2022

ea-open-source force-pushed the features/movey-increase-download-count branch from d5140e3 to ee77028 Compare August 10, 2022 02:35

awelc reviewed Aug 16, 2022

View reviewed changes

ea-open-source force-pushed the features/movey-increase-download-count branch from e6306d8 to d00aef5 Compare August 17, 2022 03:14

awelc requested a review from tnowacki August 17, 2022 17:10

ea-open-source added 8 commits August 31, 2022 09:50

Call to Movey API to increase download count

73d3eb4

Change skip_movey_call to false in test sources, added unit tests t…

cf607f5

…o `increase_movey_download_count`

Separate increase_movey_download_count from `download_and_update_if…

aa62911

…_repo`, rename `skip_movey_flag` to `skip_movey`

Refactor increase_movey_download_count unit tests

cd2f041

Refactor sleep logic of increase_movey_download_count unit tests

7a297be

Refactor comments of the wait function

2197bee

Refactor mock movey server function, rename wait function

5f0300d

- change `init_mock_server` to `mock_movey_count_request_with_response_status_code` - change `wait` to `test_thread_wait`

Rebase and rename download_and_update_if_repo to `download_and_upda…

4289293

…te_if_remote`

ea-open-source force-pushed the features/movey-increase-download-count branch from d00aef5 to 4289293 Compare August 31, 2022 03:55

awelc suggested changes Sep 9, 2022

View reviewed changes

tnowacki reviewed Sep 9, 2022

View reviewed changes

tnowacki suggested changes Sep 13, 2022

View reviewed changes

Refactor according to review

cbe1000

ea-open-source closed this Feb 17, 2023

ea-open-source deleted the features/movey-increase-download-count branch February 17, 2023 07:48

brson pushed a commit to brson/move that referenced this pull request Sep 7, 2023

NFC: Small corrections in move-mv-llvm-compiler::cli (move-language#318)

6625735

		if let Some(git_info) = &dep.git_info {
		if !git_info.download_to.exists() {

[Movey] Call Movey API to increase download count #318

[Movey] Call Movey API to increase download count #318

Uh oh!

Conversation

ea-open-source commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

How to use it

Have you read the Contributing Guidelines on pull requests?

Test Plan

Uh oh!

awelc left a comment

Choose a reason for hiding this comment

Uh oh!

ea-open-source commented Aug 17, 2022

Uh oh!

awelc commented Aug 17, 2022

Uh oh!

ea-open-source commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ea-open-source commented Aug 31, 2022

Uh oh!

awelc left a comment

Choose a reason for hiding this comment

Uh oh!

tnowacki Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

ea-open-source Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

tnowacki Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

ea-open-source Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

tnowacki Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

ea-open-source Sep 12, 2022

Choose a reason for hiding this comment

Uh oh!

tnowacki Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

tnowacki commented Sep 9, 2022

Uh oh!

ea-open-source commented Sep 12, 2022

Uh oh!

ea-open-source commented Sep 12, 2022

Uh oh!

tnowacki left a comment

Choose a reason for hiding this comment

Uh oh!

lwsinclair commented Sep 14, 2022

Uh oh!

ea-open-source commented Feb 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ea-open-source commented Jul 29, 2022 •

edited

Loading

ea-open-source commented Aug 18, 2022 •

edited

Loading