Nightly wheels #95

galleon · 2021-11-02T07:09:57Z

🚀 Motivation

As time between release can be a couple of months, it is important to publish wheels for the different supported infrastructures on a shorter term basis

🛠️ What does this PR do?

Create a nightly release once a day each time master has changed
Publish a zip containing all wheels corresponding to the change
Once a new named release is created, the nightly ones are deleted

dbarbier · 2021-11-02T08:23:21Z

You made tests it in your repo but AFAICT not with this final version, please test and provide a link.
Code is complex, but if I am not mistaken, I emulated it on my repo between v0.9.2 and master (by moving randomly master) and it would look like https://github.com/dbarbier/scikit-decide/releases/tag/nightly (each 85MB file is replaced by dummy content to not waste resources), it is impossible to know which file is the latest one.

galleon · 2021-11-02T13:43:57Z

is
Indeed I did test it but get rid of most of my tests once master was reset.
The encoding of the name is made by git on master which has a linear history. In the following pattern nightly-x-aha the higher x the more recent is your file.
It is therefore weird to have multiple similar x Can you detail how you did your tests?

dbarbier · 2021-11-02T13:50:45Z

Consider that master and nightly are at the same commit. When master moves, git describe --tags --long will print something like nightly-4-g<hash>, then nightly tag is moved to master, and so forth.

Here is the script I used; it is not robust, you may have to delete 'nightly' tag on your fork and/or local copy.

fake-nightly.py

#! /usr/bin/env python

from github import Github
import git
import os
import random
import re


def main():
    g = None
    try:
        with open(os.path.join(os.environ["HOME"], ".git-credentials")) as cred:
            for line in cred:
                m = re.match(".*//(.*):(.*)@github.com", line)
                if m:
                    user = m.group(1)
                    g = Github(m.group(2))
                    print(f"Using Personal Access Token for user {user}")
                    break
    except:
        user = "dbarbier"

    if g is None:
        g = Github()

    r = g.get_repo(f"{user}/scikit-decide")

    repo = git.Repo(".")
    commits = list(repo.iter_commits("v0.9.2...airbus/master", first_parent=True))
    commits.reverse()

    #  Create a nightly tag
    repo.create_tag('nightly', ref=commits[0], force=True)
    #  Create a nightly release
    release_nightly = None
    for release in r.get_releases():
        if release.tag_name == "nightly":
            release_nightly = release
            #  Delete previous assets
            for asset in release_nightly.get_assets():
                asset.delete_asset()
            break
    if release_nightly is None:
        release_nightly = r.create_git_release("nightly", "nightly", "These are builds generated every night using a crontab", prerelease=False)

    pos = random.randint(1, 5)
    while pos < len(commits)-1:
        sha = commits[pos].hexsha
        long_name = repo.git.describe(commits[pos], tags=True, long=True)
        print(commits[pos], long_name)
        # Create a 'nightly' tag on this commit
        repo.create_tag('nightly', ref=commits[pos], force=True)
        # Update release
        release_nightly.update_release("nightly", "These are builds generated every night using a crontab", target_commitish=sha)
        # Add a dummy file
        with open(long_name, "wt") as f:
            f.write("# Dummy file")
        release_nightly.upload_asset(long_name)
        os.remove(long_name)
        # Advance
        pos += random.randint(1, 5)


if __name__ == "__main__":
    main()

dbarbier · 2021-11-02T14:19:09Z

Even if master refers to the last release, it would not work better: https://github.com/dbarbier/scikit-decide/releases/tag/nightly2
Anyway I suggest to first publish only one asset, and when it works, to see how to keep multiple assets.

dbarbier · 2021-11-08T07:50:44Z

@galleon What does skdecide.__version__ look like?

galleon · 2021-11-08T13:18:55Z

@galleon What does skdecide.__version__ look like?

with my current env:
➜ tmp git:(master) ✗ ipython
Python 3.8.6 (default, May 10 2021, 12:38:32)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.23.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import skdecide

In [2]: print(skdecide.version)
0.9.3-dev

Starting from 3.8 one could use importlib.metadata.version but we are still supporting 3.7

dbarbier · 2021-11-08T13:35:43Z

Okay so this commit has nothing to do with nightly wheels and should be moved elsewhere.

galleon · 2021-11-08T17:07:40Z

Okay so this commit has nothing to do with nightly wheels and should be moved elsewhere.

You are right, it is connected as it can be used to track the version of an installed nightly wheel. It has been moved in an other PR.

galleon · 2021-11-08T17:17:21Z

Versioning of nightly wheels has been changed to track them using:

distance to latest release (vx.y.z)
sha number

It has been tested on galleon/master:

using a 0, 1, *, *, * crontab
commiting dummy files

In build.yml:

if nightlytag does not exist it is created
Nighly release is created or updated
New artifact is added to the release

release.yml deletes the nightly tag and release

dbarbier · 2021-11-08T17:36:10Z

Files are alphabetically sorted and not numerically sorted, which looks weird, see https://github.com/dbarbier/scikit-decide/releases/tag/nightly2

dbarbier

You assume everywhere that if nightly tag exists, then nightly release also exist (and vice-versa). This is not robust, something may be broken during previous builds. You must check against both tag and release when needed.

dbarbier · 2021-11-10T13:17:10Z

.github/workflows/build.yml

+            await github.rest.repos.createRelease({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              tag_name: 'nightly',


You could add: prerelease: true

nhuet

I guess that notebooks/05_fake_tuto.ipynb is there for testing purpose and has to be removed before merging?

nhuet · 2021-12-02T10:34:18Z

.github/workflows/build.yml

+    tags-ignore:
+      - 'nightly'


With the current filter in master branches: - "**" this is not needed as it will never be triggered on tags (which is the expected behaviour, only build for push on branches, not on any tags)

nhuet · 2021-12-02T10:52:54Z

.github/workflows/build.yml

+      - name: Continuing only if master and nightly are different
+        if: github.sha != steps.nightly.outputs.sha
+        id: continue
+        run: |
+          echo "::set-output name=value::1"
+


What is the purpose of this step? Maybe you wanted use it as a condition for each step below? But for now, they are just using the same condition ̀github.sha != steps.nightly.outputs.sha` so it looks like it is not used anywhere.

nhuet · 2021-12-02T13:19:12Z

.github/workflows/build.yml

@@ -308,3 +308,190 @@ jobs:
          commit-message: publish documentation
          clean-exclude: |
            "version/*"
+
+  upload-nightly:
+    if: (github.ref == 'refs/heads/master') && (github.event_name == 'schedule')


Did you forget to add the schedule event in "on" section ? For now it is triggering only on push and pull requests.

nhuet · 2021-12-02T13:22:56Z

.github/workflows/build.yml

+          console.log(`nightly_${distance}_${ref_head.data.object.sha.substring(0,8)}.zip`);
+
+          let release_name = `nightly_${distance}_${ref_head.data.object.sha.substring(0,8)}` + '.zip';


I would rather exchange the order and type console.log(release_name) to be sure to display what is actually used. (But not very important)

nhuet · 2021-12-02T13:24:35Z

.github/workflows/build.yml

+
+          fs.renameSync('dist.zip', release_name, function (err) {
+            if (err) throw err;
+            console.log('dist.zip was renamed to dist.zip');


Not sure what this means. Did you meant
console.log('dist.zip was renamed to ' + release_name); ?

nhuet · 2021-12-02T13:33:33Z

If I understand well this refactorying,

each night, master having changed or not, we update the nightly tag to new master position and add the new wheels to assets while keeping all other ones as well
at new release, we remove all these wheels to start again the process

Which means, between releases, we store wheels for all nights (e.g if there are 2 months between releases, we will store about 60 wheels zip). Am I correct ?

dbarbier · 2021-12-02T15:25:18Z

.github/workflows/build.yml

+            // Keep latest tag
+            for (const tag of tags.data) {
+              if (tag.name.startsWith('v')) {
+                if (tag.name > oldest_tag.name) {


This won't work (eg. v0.9.2 > v0.9.10), you need something as in versions.vue.

This has been changed using localeCompare with numeric: true

Good, I did not know about localeCompare, it works fine with multiple dots. The sensitivity argument is useless here. Please reword the commit message, there is a typo localCompare -> localeCompare.

dbarbier · 2021-12-02T15:26:41Z

.github/workflows/build.yml

+            });
+          } catch (err) {
+            console.log(err);
+          }


try/catch not needed here

dbarbier · 2021-12-02T15:30:00Z

.github/workflows/build.yml

+              page++;
+            } while (found === false && response.length > 0);
+            console.log(`Found ${distance} commits`);
+          } catch (err) {


if something went wrong we likely want to return or throw

I found a simpler solution:

// Limited to 250 commits const distance = await github.rest.repos.compareCommitsWithBasehead({ owner: context.repo.owner, repo: context.repo.repo, basehead: `${oldest_tag.commit.sha}...${master_sha}`, }).then(d => d.data.total_commits);

If there are more than 250 commits, the list of nighly wheels is not manageable and I am not sure that having wheels with the same distance won't make any significant difference.

Indeed. I learned something

dbarbier · 2021-12-02T15:33:13Z

.github/workflows/build.yml

+          } catch (err) {
+            console.log(err);
+          }
+          console.log(`Uploaded ${uploadedAsset.data.name} to GitHub`);


move this log into the try/catch clause, we do not want to display it if an error occurred

in fact this log is useless, this string is already in job output

dbarbier · 2021-12-02T15:38:38Z

.github/workflows/release.yml

+              repo: context.repo.repo,
+            })
+
+            const nightlyRelease = releases.data.find(r => r.tag_name.startsWith('nightly'))


why startsWith and not equality?

I still see it in your current branch (galleon@83b028a). BTW commit galleon@d138345d does not do what it claims.

This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Same for the second one

They are on your fork, and displayed when you click on the links; I wall update them anyway

still not fixed

dbarbier · 2021-12-02T15:40:11Z

.github/workflows/release.yml

+                repo: context.repo.repo,
+                ref: `tags/${nightlyRelease.tag_name}`,
+              })
+              console.log(`${nightlyRelease.tag_name} tag has been deleted`)


why delete nightly tag?

because there is no more nightly until the next PR

Deleting this tag or not won't change anything. Less code less bugs.

dbarbier · 2021-12-03T09:13:36Z

If I understand well this refactorying,

* each night, master having changed _or not_, we update the nightly tag to new master position and add the new wheels to assets while keeping all other ones as well

[...]
Nothing is done if master and nightly are at the same commit (test l. 369)

nhuet · 2021-12-03T09:22:48Z

Nothing is done if master and nightly are at the same commit (test l. 369)

Actually the commit for this test was added once I made the comment.

dbarbier · 2021-12-03T09:41:43Z

Actually the commit for this test was added once I made the comment.

okay this is another example why rebasing before review is finished is annoying.

nhuet · 2021-12-03T09:46:18Z

This time at least, i do not think this was the issue as the commit has been done just after my comment and has not yet been squashed or rebased (db0cb64)

dbarbier · 2021-12-06T06:11:14Z

.github/workflows/build.yml

+            owner: context.repo.owner,
+            repo: context.repo.repo,
+            ref: 'heads/master',
+          });


As mentioned in comment, there is no need for an API call, this could be written:

const master_sha = '${{ github.sha }}';

I renamed ref_head to master_sha because HEAD refers to the tip of any branch, not master.

dbarbier · 2021-12-06T06:12:21Z

.github/workflows/build.yml

+
+            if (ref_nightly.data.object.sha === ref_head.data.object.sha) {
+              console.log('No new nightly release');
+              return;


No need for console.log, you can replace by

return "No new nightly release";

dbarbier · 2021-12-06T06:13:09Z

.github/workflows/build.yml

+          console.log(`Updated ${release.data.tag_name} nightly release  ${release.data.draft} ${release.data.prerelease}`);
+
+          // Get all tags and keep the oldest one starting by v
+          let oldest_tag = { name: 'v0.0.0' };


Replace oldest by newest everywhere.

dbarbier · 2021-12-06T06:15:22Z

.github/workflows/build.yml

+            owner: context.repo.owner,
+            repo: context.repo.repo,
+            ref: 'tags/nightly',
+            sha: ref_head.data.object.sha,


It would be wise to add force: true in case master has been rebased non fast-forward.

dbarbier · 2021-12-06T06:25:52Z

.github/workflows/build.yml

+              page++;
+            } while (found === false && response.length > 0);
+            console.log(`Found ${distance} commits`);
+          } catch (err) {


I found a simpler solution:

// Limited to 250 commits const distance = await github.rest.repos.compareCommitsWithBasehead({ owner: context.repo.owner, repo: context.repo.repo, basehead: `${oldest_tag.commit.sha}...${master_sha}`, }).then(d => d.data.total_commits);

If there are more than 250 commits, the list of nighly wheels is not manageable and I am not sure that having wheels with the same distance won't make any significant difference.

dbarbier · 2021-12-06T06:27:59Z

.github/workflows/build.yml

+          fs.renameSync('dist.zip', release_name, function (err) {
+            if (err) throw err;
+            console.log(`dist.zip was renamed to {release_name}`);
+          });


Why not simply

fs.renameSync('dist.zip', release_name);

? Less code less bugs.

dbarbier · 2021-12-06T07:47:35Z

.github/workflows/build.yml

+          } catch (err) {
+            console.log(err);
+            throw new Error('Failed to get tags');
+          }


There are several occurrences of

try { ... } catch(err) { console.log(err) throw new Error(...); }

This is useless, remove the try/catch, the error message will already get displayed in log.

nhuet

LGTM. Think to squash the commits before merging.

dbarbier · 2021-12-06T14:11:11Z

.github/workflows/release.yml

+              repo: context.repo.repo,
+            })
+
+            const nightlyRelease = releases.data.find(r => r.tag_name.startsWith('nightly'))


still not fixed

dbarbier · 2021-12-06T14:14:55Z

.github/workflows/build.yml

+            });
+            console.log(`Found release ${release.data.tag_name} ${release.data.draft} ${release.data.prerelease}`);
+          } catch (err) {
+            console.log(`Release ${tag} not found`);


variable 'tag' not defined

dbarbier · 2021-12-06T14:15:39Z

.github/workflows/build.yml

+          // Keep latest tag
+          for (const tag of tags.data) {
+            if (tag.name.startsWith('v')) {
+              // if (tag.name > newest_tag.name) {


remove dead code

dbarbier · 2021-12-06T14:16:03Z

.github/workflows/build.yml

+          for (const tag of tags.data) {
+            if (tag.name.startsWith('v')) {
+              // if (tag.name > newest_tag.name) {
+              if (tag.name.localeCompare(newest_tag.name, undefined, { numeric: true, sensitivity: 'base'}) > 0) {


'sensitivity' argument is useless

dbarbier · 2021-12-06T14:16:54Z

.github/workflows/build.yml

+
+          // Upload the zip file to GitHub
+          let uploadedAsset = null;
+          uploadedAsset = await github.rest.repos.uploadReleaseAsset({


both lines can be merged: const uploadAsset =...

dbarbier · 2021-12-06T14:17:44Z

.github/workflows/build.yml

+          } catch (err) {
+            console.log(err);
+          }
+          console.log(`Uploaded ${uploadedAsset.data.name} to GitHub`);


in fact this log is useless, this string is already in job output

- retrieving artifact - storing master sha - retrieving sha for `nightly` if it does not exist create tag - retrieving release and create it if it does not exist - updating `nightly` tag - updating release - calculating distance from latest version - uploading artifact as as asset for the release - set workflow to trigger at 'deux heures moins le quart' nightly is not buildt if master unchanged This job returns the browser_download_url of the new asset

galleon self-assigned this Nov 2, 2021

dbarbier suggested changes Nov 9, 2021

View reviewed changes

dbarbier reviewed Nov 10, 2021

View reviewed changes

dbarbier mentioned this pull request Nov 12, 2021

Deployment of linters #84

Closed

10 tasks

nhuet reviewed Dec 2, 2021

View reviewed changes

dbarbier suggested changes Dec 2, 2021

View reviewed changes

dbarbier reviewed Dec 6, 2021

View reviewed changes

nhuet approved these changes Dec 6, 2021

View reviewed changes

dbarbier reviewed Dec 6, 2021

View reviewed changes

galleon added 2 commits December 6, 2021 15:57

Deleting nightly release once new v release published

82568c7

dbarbier approved these changes Dec 6, 2021

View reviewed changes

galleon merged commit b4aca1c into airbus:master Dec 7, 2021

galleon deleted the gal/nightly_wheels branch December 15, 2021 18:37

galleon mentioned this pull request Dec 16, 2021

Nighly wheels generation #77

Closed

		console.log(`nightly_${distance}_${ref_head.data.object.sha.substring(0,8)}.zip`);

		let release_name = `nightly_${distance}_${ref_head.data.object.sha.substring(0,8)}` + '.zip';

Nightly wheels #95

Nightly wheels #95

Conversation

galleon commented Nov 2, 2021

🚀 Motivation

🛠️ What does this PR do?

dbarbier commented Nov 2, 2021

galleon commented Nov 2, 2021

dbarbier commented Nov 2, 2021 • edited Loading

dbarbier commented Nov 2, 2021

dbarbier commented Nov 8, 2021 • edited Loading

galleon commented Nov 8, 2021

dbarbier commented Nov 8, 2021

galleon commented Nov 8, 2021

galleon commented Nov 8, 2021

dbarbier commented Nov 8, 2021 • edited Loading

dbarbier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhuet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhuet Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhuet commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier commented Dec 3, 2021

nhuet commented Dec 3, 2021 • edited Loading

dbarbier commented Dec 3, 2021

nhuet commented Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhuet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbarbier commented Nov 2, 2021 •

edited

Loading

dbarbier commented Nov 8, 2021 •

edited

Loading

dbarbier commented Nov 8, 2021 •

edited

Loading

nhuet Dec 2, 2021 •

edited

Loading

dbarbier Dec 6, 2021 •

edited

Loading

nhuet commented Dec 3, 2021 •

edited

Loading

nhuet commented Dec 3, 2021 •

edited

Loading