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

Update whitelist.txt #5

Merged
merged 4 commits into from
Sep 6, 2024
Merged

Conversation

egrace479
Copy link

Preliminary whitelist with all container contents and baseline submission requirements.

preliminary whitelist with all container contents and baseline submission requirements
egrace479 added a commit to Imageomics/HDR-anomaly-challenge that referenced this pull request Aug 19, 2024
update following finalization with A3D3: a3d3-institute/HDRchallenge#5
expand versioning options for whitelist
@egrace479
Copy link
Author

I added the recommended version regex from PyPI (it's more expansive) as suggested by @work4cs for the whitelist test as well.

egrace479 added a commit to Imageomics/HDR-anomaly-challenge that referenced this pull request Aug 21, 2024
Add more packages suggested in a3d3-institute/HDRchallenge#5

Co-authored-by: Jiaman Wu <47147576+work4cs@users.noreply.github.com>
work4cs added a commit to Imageomics/HDR-anomaly-challenge that referenced this pull request Aug 23, 2024
* Update ingestion code to check model requirements are in whitelist before installing

* Add preliminary whitelist
update following finalization with A3D3: a3d3-institute/HDRchallenge#5

* Update ingestion_program/ingestion.py

use pypi's full versioning regex

Co-authored-by: Jiaman Wu <47147576+work4cs@users.noreply.github.com>

* Update whitelist.txt

Add more packages suggested in a3d3-institute/HDRchallenge#5

Co-authored-by: Jiaman Wu <47147576+work4cs@users.noreply.github.com>

---------

Co-authored-by: Jiaman Wu <47147576+work4cs@users.noreply.github.com>
update the whitelist to match change on imageomics

https://github.com/Imageomics/HDR-anomaly-challenge/blob/2e54051b587d0a890c426512fa3bd177b8b0a753/ingestion_program/whitelist.txt

Co-authored-by: Lisa <47147576+work4cs@users.noreply.github.com>
Copy link
Member

@ytchoutw ytchoutw left a comment

Choose a reason for hiding this comment

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

Add packages for iHarp

gw_challenge/ingested_program/whitelist.txt Show resolved Hide resolved
@ytchoutw
Copy link
Member

ytchoutw commented Sep 6, 2024

Shall we merge this already and update it to Codabench? I think we have most of the required packages

Co-authored-by: Yuan-Tang Chou <ytchou97@gmail.com>
@egrace479
Copy link
Author

We can always add more later, as we did set it up to tell participants to request unavailable packages through GitHub.

@katyagovorkova katyagovorkova merged commit 0d21b55 into a3d3-institute:main Sep 6, 2024
1 check failed
@@ -8,7 +8,7 @@


# expected version pattern for requirements
VERSION_PATTERN = re.compile("^[0-9].[0-9].[0-9]$")
VERSION_PATTERN = re.compile("^[N!]N(.N)*[{a|b|rc}N][.postN][.devN]$")
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry I only saw it after merging, but upon testing, this version pattern doesn't seem to be doing what we want? For example, version "2.15.0" does not match the pattern, is it expected behaviour? Thanks

@egrace479 @work4cs

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that does not seem to be working. @work4cs, this was your update on ours, could you please resolve this or revert both back to the pattern I initially set up? Thanks!

Copy link

Choose a reason for hiding this comment

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

I may not revert to the old version. A period mean anything. So, "1@2@3" will also match. Instead, I will convert this ^[N!]N(.N)*[{a|b|rc}N][.postN][.devN]$ into python regex verion.

Copy link

@work4cs work4cs Sep 6, 2024

Choose a reason for hiding this comment

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

We can use package

from packaging.version import Version Version('1.2@3') #raise an exception Version('1.2.3') #return an object
if not Version(version) will be enough

Copy link

Choose a reason for hiding this comment

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

I agree, that seems like the right thing to use rather than come up with our custom reinvention. Nice find @work4cs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@work4cs interesting solution! Could you please add it to the code and make another pull request? Thanks!

Copy link

Choose a reason for hiding this comment

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

I updated butterfly challenge. Not sure how to update here. Which repo should I update, egrace479:HDRchallenge or a3d3-institute:HDRchallenge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for pointing out to the changes, I will transfer them to this repo!

Copy link

@work4cs work4cs Sep 10, 2024

Choose a reason for hiding this comment

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

I wrote down and tested two ways to handle the situation of the version does not match the regex:

  1. throw exception and exit (currently using)
  2. catch the exception of error, print the error message and install the default version (commented out)
    I currently use 1 because the 2's message is easily ignored among all the installing messages in the console. And if the default version we automatically install for them does not work, it might be hard for the participants to figure out we install another version and it causes some errors.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds like a good plan @work4cs, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants