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: Support globs in MatchSpec build strings #3735

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Jan 8, 2025

Fix #3699.

@jjerphan jjerphan force-pushed the fix/correct-regexspec branch 2 times, most recently from 3ef58f7 to c7db207 Compare January 8, 2025 16:31
@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Jan 8, 2025
@jjerphan jjerphan force-pushed the fix/correct-regexspec branch 2 times, most recently from 6f965bc to 844e4bc Compare January 8, 2025 16:55
@jjerphan jjerphan marked this pull request as ready for review January 8, 2025 18:07
libmamba/include/mamba/specs/regex_spec.hpp Outdated Show resolved Hide resolved
libmamba/src/specs/regex_spec.cpp Outdated Show resolved Hide resolved
Comment on lines +47 to +60
// Construct ss from raw_pattern, in particular make sure to replace all `*` by `.*`
// in the pattern if they are not preceded by a `.`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this mean that a glob string py3.* would match py310 when it shouldn't have? i.e. we should first escape the existing . to \., and then replace * by .*?

Copy link
Member Author

@jjerphan jjerphan Jan 9, 2025

Choose a reason for hiding this comment

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

Yes, I am trying to think of other cases convertions to regular expression might have changed (e.g. pattern with ?).

I will add more test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Arf, I support your remark, but this clashes with behavior:

TEST_CASE("ChimeraStringSpec py.*")
{
auto spec = ChimeraStringSpec::parse("py.*").value();
REQUIRE(spec.contains("python"));
REQUIRE(spec.contains("py"));
REQUIRE(spec.contains("pypy"));
REQUIRE_FALSE(spec.contains(""));
REQUIRE_FALSE(spec.contains("cpython"));
REQUIRE(spec.str() == "^py.*$");
REQUIRE_FALSE(spec.is_explicitly_free());
REQUIRE_FALSE(spec.is_exact());
REQUIRE_FALSE(spec.is_glob());
}

Copy link
Contributor

@jaimergp jaimergp Jan 9, 2025

Choose a reason for hiding this comment

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

In conda, the "full regex mode" only kicks in if the build string is wrapped in ^...$. Otherwise, it's understood as a normal glob string. See docstring and implementation.

Comment on lines 90 to 97
TEST_CASE("RegexSpec * semantic")
{
auto spec = RegexSpec::parse("py3.*").value();

REQUIRE(spec.contains("py3."));
REQUIRE(spec.contains("py3.10"));
REQUIRE(spec.contains("py3.10_cuda11.8_cudnn8.7.0_0"));

REQUIRE_FALSE(spec.contains("py3"));
REQUIRE_FALSE(spec.contains("py310"));
}

TEST_CASE("RegexSpec ? semantic")
{
auto spec = RegexSpec::parse("py3.?").value();

REQUIRE(spec.contains("py3."));
REQUIRE(spec.contains("py3.1"));
REQUIRE(spec.contains("py3.0"));
REQUIRE(spec.contains("py3.?"));
REQUIRE(spec.contains("py3.."));

REQUIRE_FALSE(spec.contains("py3.10"));
REQUIRE_FALSE(spec.contains("py3"));
REQUIRE_FALSE(spec.contains("py310"));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is the semantics we want to use, yet it clashes with ChimeraString's (see https://github.com/mamba-org/mamba/pull/3735/files#r1908971307).

@jjerphan jjerphan marked this pull request as draft January 9, 2025 15:47
@jjerphan jjerphan force-pushed the fix/correct-regexspec branch from b8104d2 to 2d1ce12 Compare January 15, 2025 14:58
@jjerphan jjerphan force-pushed the fix/correct-regexspec branch from 2d1ce12 to 1dacead Compare January 24, 2025 10:33
jjerphan and others added 5 commits January 24, 2025 11:34
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>

Co-authored-by: Johan Mabille <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>

Co-authored-by: jaimergp <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>

Co-authored-by: jaimergp <[email protected]>
@jjerphan jjerphan force-pushed the fix/correct-regexspec branch from 1dacead to 5a33edd Compare January 24, 2025 10:35
@jjerphan jjerphan marked this pull request as ready for review January 24, 2025 10:47
m_raw_pattern = raw_pattern;
m_pattern = std::regex(m_raw_pattern);
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would find it clearer to have something like:

RegexSpec::RegexSpec(std::string raw_pattern)
    : m_raw_pattern(format_raw_pattern(std::move(raw_pattern))
    , m_pattern(std::regex(m_raw_pattern))
{
}

And have all the logic in format_raw_pattern (or any better name you can come with) rather than in the constructor, with a return statement.

Copy link
Member Author

@jjerphan jjerphan Jan 24, 2025

Choose a reason for hiding this comment

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

I thought of this, but does this guarantee that m_raw_pattern is strictly initialized before m_pattern?

I am not sure it is safe as m_pattern is declared before m_raw_pattern:

std::regex m_pattern;
std::string m_raw_pattern;

Copy link
Member

@JohanMabille JohanMabille Jan 24, 2025

Choose a reason for hiding this comment

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

Ah good catch, you have to invert them in the declaration. The intialization order is guanranteed to be the same as the declaration order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could this change the ABI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::bug_fixes For PRs fixing bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2 doesn't solve pytorch=2.3.1=py3.10_cuda11.8*, but v1 does
4 participants